Add new path finding command 2001

[ell1e]

This adds a new path finding command with initial code by @MackValentine that I fixed up, and @jetrotal helping out coordinating the whole thing. It's based on the new CheckWay command added in #3120 and #3130 and can be used via event command 2001. It has a few special options which are documented in the doc comment that allow ignoring other events in the pathing (they won't be ignored in the actual collision!). The command is intended to both work for a use case where it's used once and then sets a movement route that is just left to execute, as you would in an autorun event, and for a use case where it's used repeatedly in a parallel event to continuously update the route to a moving target.

Behavior quirks: The current algorithm is a breadth first search, so it's not full A* yet. I might update it to an A* later if I get around to that, for now it seemed more relevant to get it to work properly with all the options people may want to use. It uses a search cutoff to avoid huge lag even when used constantly in parallel events by multiple moving things. Whenever a target is too far away to be safely located within that cutoff the command will set a guessed, approximate route. This means if the target is too far away the search might get lost and route into wrong sections of the map, this isn't a bug.

Another edit/additional note: I should be available also in the longer term if something with this code breaks to look into it.

[ell1e]

It seems like Debian's presumably old g++ doesn't like the exact way the hash function looks like. I suppose way way to fix it would be to no longer nest it inside the SearchNode definition, but for clarity reasons I actually like that layout. I wonder if there's a different way to appease it? I'm not quite sure I fully understand its error message either. If any C++ expert knows how to get it to work while still keeping the hash function inside the SearchNode struct, lemme know!

[ell1e]

Converting to draft for now since there seems to be a path finding bug remaining where the path isn't found when it should be. jetrotal is helping me debug, should hopefully have a fix in a day or two.

[ell1e]

I suppose way way to fix it would be to no longer nest it inside the SearchNode definition

Nope, didn't fix it. I'm actually clueless why the Debian 10 g++ is unhappy then. If anyone knows, happy for pointers.

[jetrotal]

@Ghabry Since you commented soon you'll have some time to check this one...
This one is near completion, only two extra things:

• First one is the Debian compatibility with Search Node.

• Second is related to lag in my end when multiple events use Path Finding.
  Adding if (!event-IsMoving()) return false; right after event is defined helped me,
  But Ellie commented about extra issues that would cause:
  

[ell1e]

Regarding lag, you could alternatively for example add a sleep/wait into your parallel event to not run the path finding every frame. Of course that means sometimes it'll set the route with a slight delay but it shouldn't be too bad. (And it would then still react to doors, albeit also with sometimes a delay.)

As for Debian, the main reason I haven't done anything about it yet is that I'm simply clueless so far what even the problem is. I tried multiple variants to specify the set hash function, debian 10's g++ doesn't like any of them. I'm really not sure why it doesn't like them, the error suggests a wrong type or wrong function signature for the hash function to me, but I can't really figure out why it claims that. Maybe it's a compiler bug? I'm not sure how to address it.

[ell1e]

For what it's worth, @jetrotal suggested some behavioral changes that could make this more intuitive to use and less performance intensive, and there were some pretty good ideas that I still have to test out. But the plan is that I test with the actual 2K3 editor first and compare how other movement route commands behave. Once I got that all checked out and adjusted, I'll undraft this again. (I'm assuming it's better to figure this out now before merging, so that people don't start to use it already and then the behavior suddenly unnecessarily changes with a later update.)

However, there shouldn't be any massive changes upcoming anymore, so if anyone wants to have an initial look at the overall code quality now wouldn't be a bad time.

[jetrotal]

Hi Ellie, thanks for working and improving this 🙏
Reviewing updates is a bit slow right now, since we have a lot of critical stuff on the next version checklist: https://github.com/EasyRPG/Player/milestone/26

But, I'd say your pr is looking good, and I'm willing to keep testing it while its being updated

tnx again!

[Ghabry]

Hey. Please don't feel discouraged when their is no feedback for a while.

We are close to 0.8.1 release and want to get most of the stuff sorted out first before I look at the "lower priority" stuff like your new features.

I like this new command but right now cannot look more at it, sorry.

[ell1e]

No problem, no need to rush. I might also be busy for the next 3-4 weeks or so, so the final changes might also take me a bit. I won't be going anywhere if this still takes a bit, no worries.

[ell1e]

After some discussion I propose a split into two separate commands:

"Smart Move Route" (2001):

/* This is the "Smart Move Route" command:
 * Like "Set Move Route", this command sets a longer route
 * of all the steps necessary to a possibly farther off target.
 * The route is computed to smartly go around any obstacles.
 * This command is best used in "Autorun" blocking events, e.g.
 * in situations where an event or character should be sent on
 * their way with a single command. Don't use it every frame in a
 * "parallel" event because it uses a higher search depth and will
 * cause extreme lag if used so often, use "Smart Step Toward" for
 * parallel events instead.
 *
 * Event command parameters are as follows:
 *
 * Parameter 0, 1: Passed to ValueOrVariable() to get the moving event's ID.
 *
 * Parameter 2, 3: Passed to ValueOrVariable() to get the target X coord.
 *
 * Parameter 4, 5: Passed to ValueOrVariable() to get the target Y coord.
 *
 * Parameter string: Allows free form options, see generic
 * CommandSmartMoveRoute() function prototype for full list.

and this one:

"Smart Step Toward" (2002):

/* This is the "Smart Step Toward" command:
 * Unlike "Set Move Route" that always applies even if the event or
 * character is currently between two tiles and moving, "Smart Step Toward"
 * tries to be economical and doesn't change anything if the event is
 * currently moving, to avoid expensive comptuations.
 * Otherwise, it sets a new one-step move route, computed to smartly move
 * one tile toward a given target. Because of it's economical behavior,
 * this command is useful for "Parallel" event use in every frame to
 * track a moving target continuously.
 * Event command parameters are as follows:
 *
 * Parameter 0, 1: Passed to ValueOrVariable() to get the moving event's ID.
 *
 * Parameter 2, 3: Passed to ValueOrVariable() to get the target X coord.
 *
 * Parameter 4, 5: Passed to ValueOrVariable() to get the target Y coord.
 *
 * Parameter string: Allows free form options, see generic
 * CommandSmartMoveRoute() function prototype for full list.

[ell1e]

Since I saw this pull request mentioned elsewhere, I'm still interested in helping out with getting it across the finish line once there's a good opportunity. It's mostly a draft since I'm hoping for more input.

[jetrotal]

Here's one suggestion (idk if there's time to suggest until 8.1 release)...

A parameter to disable diagonal path finding:

• If a dev wants to imitate the look and feel of older RPG, this dev may want to use only Up, Down, Left and Right movements.

[ell1e]

A parameter to disable diagonal path finding

I like it, I added a first test version but I didn't test if it compiles. Still waiting for liblcf for half an hour or so, then I'll fix it if it doesn't.

[Ghabry]

about error: duplicate case value

There are now enums available:

EasyRpg_SmartMoveRoute and EasyRpg_SmartStepToward which maps to 2003 and 2004. So the numbers changed. Cannot remember why I did this, probably because TriggerEventAt is at 2002 already (?)

[ell1e]

It should now build again, and I added a description of allowDiagonalMovement=0 as a potential option into the doc comment.

[ell1e]

@jetrotal would you be up for testing how the diagonal movement disabling acts like? I remember you having a few useful test maps.

[Ghabry]

@ell1e sorry for the slow feedback here. I'm still working on getting this in for 0.8.1.

Just to make the meaning of maxSearchSteps and maxRouteSteps more clear (correct me if my explanation is wrong).

Search steps defines the upper bound of the iterations the pathfinding algorithm can do before it gives up. Reducing this number can improve the performance but can fail to reach the goal.

After the pathfinding finishes the route steps defines how many tiles (upper bound) from the current location the event will move towards the target. Reducing this number does not affect the performance as this only truncates after the path finding algorithm finished.

[ell1e]

Yes, that's correct. The main use case for maxRouteSteps is to do a single step e.g. in a parallel event, and the main use case for maxSearchSteps is to configure a performance vs. quality trade-off.

Edit: and since there's a separate command for a single step, the normal user probably will never use maxRouteSteps manually. But I thought why not leave it available for some weird use case I'm not thinking of, since the option is needed internally anyway. (And "Smart Step Toward" 2002 won't replace the route if the character is already moving at the time, but "Smart Move Route 2001 with maxRouteSteps=1 would, so I guess there's technically still a different use case possible.)

[Ghabry]

Okay while you are around I directly have a second question about the semantics of abortIfAlreadyMoving.

if (abortIfAlreadyMoving && event->IsMoving())
    return false;
CancelMoveRoute();

Based on your implementation:

When abortIfAlreadyMoving is 0: It cancels an active movement and does the pathfinding (makes sense)

When its 1 and the Event is not moving: Same as 0

When its 1 and the Event is moving: The code returns false. This means the current interpreter will yield and next frame try run the same event command again. And this repeats until the event is not moving.

Is that intentional?

[ell1e]

Is that intentional?

No, that's a bug. The command is supposed to act like it completed successfully. Thanks for catching it, I'm not very familiar with how the interpreter works. Would it need to return true then? If you want and can, you can just change the commit accordingly and force push, or I can do it if you prefer me to.

Edit: it makes me wonder though, if abortIfAlreadyMoving should be renamed doNothingIfAlreadyMoving. And perhaps we should get @jetrotal to test the fixed version just to make sure the parallel use keeps working as intended.

[Ghabry]

Okay I thought it's maybe intentional to make the timing for One Step Forward simpler. This way you can just make the command "hang" until the event is ready to be moved again. I'll think about it.

[ell1e]

to make the timing for One Step Forward simpler.

I'm pretty sure you could still do that by just using the 2001 Smart Route with maxRouteSteps=1 which would replace the route entirely, but effectively this still waits until the current tile movement is completed. This is I think what "One Step Forward" would do if issued as a set movement route.

[Ghabry]

hm, you are right. I'll do the further commits as I also relocated some of the code to Game_Character so when you push anything it will be full of conflicts.

[jetrotal]

One thing that is important to inform, is that ghabry plans to rewrite all the syntax to use parameters only instead of the string parsing currently in use.

That due to the UI being responsible of the management of the code, instead of the string data:

[Ghabry]

As already preannounced by Jetrotal I replaced the string parsing with integer parameters as it is now possible to write our own editor plugins.

"Smart Move Route" and "Smart Step Forward" were unified into one command "Pathfinder". Instead the values for these two are provided as templates via buttons in the editor interface (with the values you suggested).

The pathfinding code itself was relocated to Game_Character but besides some naming convention stuff I didn't alter the code.

---

Summary: The argument parsing was replaced, the pathfinding algorithm itself is the same. Thanks.

---

Plugin for latest Maniac Patch Editor

EasyRpgCommands.zip