Walk animation keeps going after character arrived at destination

[ericoporto]

Observed by Francisco in higher resolution games, when character walks all the way from right to left in some of his rooms, with movement linked to animation, once the character has arrived at destination, the animation still goes for a few frames before stopping. Not sure if relevant but he also reported using 1:2 mask resolution. He also noted he has observed the glitch before at AGS 3.4.

[ghost]

Francisco's gif: https://cdn.discordapp.com/attachments/221048075207180288/628636698913538049/walkinplace.gif

[ivan-mogilko]

Not sure if it's the same issue, but there's a confirmed bug when on-going animation may reset to frame 0 and play again as soon as character stops: #1123

[ivan-mogilko]

By the way, I did a fix where a character was stuck in either a "walking" state or "between" states for a short while, before going idle. I wonder if this is related somehow:

1084cf5

[ivan-mogilko]

@AlanDrake do you know if we are allowed to attach the test game from Franscisco, the one that you sent to me, as a test case which reproduces 100%?

I wonder if it will still reproduce if we replace game gfx with placeholders, as this likely is not related to sprites.

[AlanDrake]

I don't remember, but I guess placeholders should work, only thing I'm not sure is if the number of frames have an effect over this bug.

[ericoporto]

I mean, if it's for others to have you can put on a filesharing and just pass to the others on Discord to test, we aren't that many.

[ivan-mogilko]

This is for the record, because there has to be a reproducible test case in the ticket.

[ivan-mogilko]

So, testing this under debugger, what happens in the code is this.

The algorithm in question is do_movelist_move function:

ags/Engine/main/update.cpp

Line 64 in f4bd80d

int do_movelist_move(short*mlnum,int*xx,int*yy) {

Specifically, what happens when the character finished walking along one axis, but did not finish walking along another axis. For instance, it reached X destination, but not Y:

ags/Engine/main/update.cpp

Lines 73 to 81 in f4bd80d

	if (cmls->doneflag & 1) {
	// if the X-movement has finished, and the Y-per-move is < 1, finish
	// This can cause jump at the end, but without it the character will
	// walk on the spot for a while if the Y-per-move is for example 0.2
	// if ((ypermove & 0xfffff000) == 0) cmls->doneflag|=2;
	// int ypmm=(ypermove >> 16) & 0x0000ffff;
	// NEW 2.15 SR-1 plan: if X-movement has finished, and Y-per-move is < 1,
	// allow it to finish more easily by moving target zone

Then the engine does the following:

1. Sets flag "x done", locking x position (won't be updated further);
2. Tries to "hack" the Y destination, by moving it closer to the current position. Right, moving the destination, not the character. This hack is only 1-3 pixels, so if the distance is bigger, then the character will still not finish walking.
3. If Y pos is still not at the destination, then increments onpart and waits for the next frame to repeat the process.

By the way, notice there are 2 overriding comments in the linked code above. First CJ wanted character to "jump" to destination, but instead decided to "move" destination instead.

Now, there are few nuances in this:

1. This "destination hack" is not saved, meaning next time the destination is far again, and engine repeats same exact fix.
2. Since onpart only advances the y pos by a fraction of a pixel, it takes many frames for Y pos to actually move a single pixel closer to the destination.
3. If there's also "movement linking to animation", then this update is done not each frame, but once in N frames (depending on the animation speed, frame delays etc).

This makes the process longer, and this visible effect happens.
And yes, because of the hack, it also means that the character may end up on a slightly wrong Y pos.
Possibly same may happen in the inverse case (X becomes Y) if walking is done along almost vertical line.

The reason for the coordinate mismatch is likely a relatively low precision of fixed-point values, compared to e.g. floats.

[ivan-mogilko]

So, my guess is that there are 2 solutions:

1. Using float math, as it's much more accurate, and will likely make X/Y relation precise enough. Then presumably the destination mistake will never be large, possible 1-2 pixels tops, and this hack may be replaced with a simple "jump" to dest.
2. If not p1, then invent something to make final Y step faster.

In regards to p1, knowing history of AGS, there's a above-zero chance some old game was scripted to detect character at very particular positions...

[ivan-mogilko]

For p2: for example, adding following code in this function reduces the "walking on the same spot" considerably:

  // Update target in case it was hacked in any of the above cases
  cmls->pos[cmls->onstage + 1] = ((targetx & 0xffff) << 16) | (targety & 0xffff);

But this will also make character end up slightly further from destination. 6 pixels diff in the case of Francisco's test game, but I guess this is not overly critical for a high-resolution game.

[AlanDrake]

Well, p1 is already done, so it could be backported to 3.6 I guess.

[ivan-mogilko]

The small fix posted above solves the issue.
But i also want to investigate porting floats solution, as I also wonder if that may fix #1879.

Besides that, it was interesting to examine this code, as it contains old variants of logic in comments. I think it's worth to take it out and make a combined "history of changes" above the function, for convenience.

Specifically noticed this piece of code:

ags/Engine/main/update.cpp

Lines 132 to 136 in f4bd80d

	// if the Y is almost there too, finish it
	// this is new in v2.40
	// removed in 2.70
	/*if (abs(yps - targety) <= 2)
	yps = targety;*/

I immediately remembered this ticket: #663
may be worth checking that out too.

[ivan-mogilko]

Well, p1 is already done, so it could be backported to 3.6 I guess.

I've been thinking about porting floats, and then I recalled there already had been a backwards compatibility issue when we replaced a pathfinder. Which means that maybe it's best to stick with the existing math in the compat. branch.
I guess we could have both float and fixed point stored in that struct depending on game format, but that might increase complications, so i'd leave that as a backup idea. I guess if someone would want to support that, they would better use 2 different MoveList structs, and 2 variants of do_movelist_move function, one for fp math and another for float math.

[ivan-mogilko]

In terms of possible move corrections, there are 2 approaches: 1) accept that the final destination is inaccurate and just stop where we are; or 2) walk part or all of the remaining destination along 1 axis. In other words: correctness vs making it "look nicer".
Current code does the latter, but it causes this walking in place error if the position mistake is bigger than expected. What is causing this is that the velocity angle is turned away from the axis in question.

I came to another fix idea, which hopefully will be compatible. Since the problem is that the speed along that other axis is not big enough, my idea is to create a "extra step" state in that function, which would in that case "turn the velocity vector", so to speak, and make character move remaining distance fullspeed along the remaining axis.

In terms of code, this looks roughly like this:

yps = cmls->fromy + (int)(fixtof(ypermove)*(float)cmls->onpart) + // main move
          (int)(fixtof(fin_ymove)*(float)fin_onpart); // final move

in other words, the current position would consist of 2 parts: the regular move part, and the "final" move part which is added only in case of this unfortunate situation (otherwise it stays zero).

I noticed that in high-resolution games the behavior is almost indistinguishable from simply "jumping" to the destination, as the diff is too small relatively to the screen.

However, when testing the low-res game (320x200), i found that the jump will actually be quite noticeable. While the fix I am trying here visibly looks like an extra step in a new direction.

So, I would really like to know opinion on this looks visually.

Here's the test game I compiled:
https://www.dropbox.com/scl/fi/r52c6zy1odiabeuwnautu/walk-problem-test.zip?rlkey=3e9yfgeze8dujeeb1n4ov6d3u&dl=0
and a project, in case necessary:
https://www.dropbox.com/scl/fi/4qngs3cdm58601stbb8qx/walk-problem-project.zip?rlkey=89529mos1kyc3cft7h70u9p4u&dl=0

I scripted this game to make a walks between very particular coordinates that illustrate this problem case: this is done by pressing Space button.
But if you walk manually around, you might also get similar cases. Usually the final "step" is smaller though.

---

If you think that above looks bad, I guess a remaining option would be to increase the allowed destination mistake for hi-res games. Because the 2-3 pixels of allowed diff is only suitable for the low-res.

[ericoporto]

Ideally, if there is a path to the destination, it would be nice if the ending position is exactly the destination, because people usually script a check using equality (player.x == DEST_X && player.y == DEST_Y). If script coordinates were float, maybe people would be using some epslon for the tolerance.

So, I would really like to know opinion on this looks visually.

Only in the specific case with pressing space I can note that the player kinda "slides in", maybe because you mentioned, but any other clicking around I didn't noticed.

---

Btw, if this changes the final position, while it's good for newly created games, this can still impact the older ones, so if there is work to keep the old ways for old games, then maybe it's similar work than backporting the float version - supposing the float version has this magically fixed. But if it still makes the character end in the wrong position (even if it's valid), then it may be better to go the other way (and later adjust it in ags4).

About the strategy of moving the destination, this can be simulated at the time the Walk command is done, so that the player Destination property has the correct value from the start.

[fernewelten]

Ideally, if there is a path to the destination, it would be nice if the ending position is exactly the destination, because people usually script a check using equality (player.x == DEST_X && player.y == DEST_Y)

A bigger problem of inexact stopping would be that the end point of the walk might happen to be at the very edge of a walkable area. When an automatically scaled character“overshoots” for their last step and moves off the area, and the edge of this area would have the character at, e.g., 20 % size, then this overshooting makes the character suddenly become five times as big as it should be.

When inexact walking makes the character stop outside a region instead of on the region, then the event “Steps onto” won't trigger.

The player themselves would have the expectation, especially in a low-res game: “I clicked with my mouse exactly there, and so I want Ego to move exactly there instead of some pixels to the side.“

[ivan-mogilko]

@ericoporto

Btw, if this changes the final position, while it's good for newly created games, this can still impact the older ones

This is true, but I might also keep the destination fixup for the old games. The old fix and this new solution do not conflict with each other. In case of a destination fix, the "final step" will simply be shorter, or zero, depending on a case.

In fact, seeing how there are different versions of the code commented around this function, it's quite plausible that someone will have to uncomment and optionally support these too, for some particular very old games.

[ivan-mogilko]

In regards to ags4, now when I understand all this code better, it seems all or most of these fixups are not necessary at all, and if it's true, then this whole function may be stripped to a minimal code.