Fix formula used for acceleration

[appgurueu]

"Breaking" change. Fixes #12340.

Note: Jumping etc. necessarily feels slightly different with this the lower the FPS (hence why this is a breaking change); some defaults such as gravity should be considered to be tweaked.

[lhofhansl]

It's definitely going to be more accurate then before and less prone to rounding error.
So I'm a fan.

Need to test it, though.

Update: It all feels OK. I'd be OK with merging this. But this does not replace rigorous testing. Perhaps some platform or parkour games might be broken now.

[Wuzzy2]

Perhaps some platform or parkour games might be broken now.

This alone should be reason for immediate rejection, especially for a minor release.

Breakage is likely because sometimes, jumps are very close. This is a very major change for a minor release that claims to preserve compatibility. Such major movement changes should NOT be taken lightly. @sofar will probably not like this either because he can then basically kiss good-bye to all the parcours boxes in Inside The Box as well as related stats which need to be invalidated. And it's not just parkour maps, it will also break any other game in which the jumping height has been fine-tuned, so it's possible that jumps that were easy before are now impossible.
This amount of breakage is definitely unacceptable for a minor release, and it's questionable even for a major release.

As long this change does not come with any form of compatibility, this is a BIG thumbs down from me.

[Wuzzy2]

Addendum: It's unfortunate acceleration is so broken right now, but breaking existing games is also not good. So I understand why this PR was made. So IMHO if this is merged, the next release MUST be 6.0.0 because the implications of this are massive. It WILL be painful for servers, games and mods to clean up, but at least they were warned.

[appgurueu]

Would you be happy with an (by default unset) object property for a smoother transition? Things to note:

• This should increase jump height as the previous calculation resulted in stronger acceleration;
• On clients the error may be negligible as they often use very small step sizes (FPS) - the higher the stepsize, the smaller the error

[Wuzzy2]

Object property sounds reasonable but then this also has to be supported till eternity.

I'm not a physics expert, but I recommend to make a full review of the object physics then, to see if there is anything else that is very broken. If you're going to change physics at all, do it right the first time. Multiple incremental breaking changes are worse than one big breaking change that does everything right from the start.

[lhofhansl]

Maybe tag this for 6.0.0 then.

[appgurueu]

This might be added to the breakages list by a core dev. I'm not opening a second PR for that though as to not clutter up the PR list.

[SmallJoker]

Hot take: This is not a breaking change.

Why? While testing #11367 I noticed that Minetest 5.2.0 is the last version where 1 full node + a 0.5m slab could be jumped on. The breakage already happened, just nobody noticed. Also while testing this PR for a bit, it does not seem to be much different (running at 60 FPS).

[appgurueu]

Hot take: This is not a breaking change.

Why? While testing #11367 I noticed that Minetest 5.2.0 is the last version where 1 full node + a 0.5m slab could be jumped on. The breakage already happened, just nobody noticed. Also while testing this PR for a bit, it does not seem to be much different (running at 60 FPS).

As I state in my analysis on the issue:

The percentual error with constant stepsizes d after a time t has passed is d/t

The larger the stepsize (i.e. "lag"), the larger the error but also the shorter the time, the larger the error (which is why jump heights are off)

On clients with high FPS (30 - 60 FPS) the error will thus already have been acceptably small for this to qualify as non-breaking. In fact this increases fairness among clients, since all clients fall at the same speed now (without this PR clients with higher FPS would fall slower due to the error being smaller). What concerns me more are servers which operate at a stepsize of 10 steps per second. During the first couple steps the error is rather significant there.

Note also that this should allow you to jump (a very tiny bit) higher than before since before gravity was over-accounted for. Parkours etc. should still work just fine. I recommend you to try it out, f.E. by playing with this PR on AES and attempting the "Tower of Gods". If anything it got slightly easier, but presumably it would be much easier using 5.2 anyways.

[SmallJoker]

Spreadsheet for comparison: server_acceleration_calc.ods << formulas might not show up properly on MS Excel.

Using the default server step (assuming constant), the difference is 0.28 nodes at the jump's peak height.

EDIT:

v0 = 6.5
g = 9.81

h = 1/2 * g * t²
v0 = g * t  ->  t = v0 / g

h = 1/2 * v0² / g = 2.153

Did I miss something? The player should jump 2.15 nodes high, but does not do that.

[pecksin]

Another peculiarity of jumping in versions < 5.3 that seems to not be well-known is, due to a larger ground collision tolerance, a sufficiently fast client stepsize can result in immediately (repeatedly?) jumping again while still inside that tolerance, resulting in inconsistently higher jumps. (https://github.com/TheTermos/minetest/blob/faac0f8e693d99cf1ffa80dfe201990647c53c72/src/collision.cpp#L567)

More noticable with trampolines (< v5.3) since bounce accumulation is applied with each jump.

[appgurueu]

Did I miss something? The player should jump 2.15 nodes high, but does not do that.

Here's my calculation:

For the gravity: $v = gt$

For the jump speed: $v = v_0$

At the highest point of the parabola, the vertical component is zero - e.g. the downwards velocity added through gravity equals the initial velocity: $v_0 = gt$ thus $t = \frac{v_0}{g} = 6.5/9.81 = 0.663 [s]$. That's the point in time. Now we may obtain the height as $s = v_{0}t - \frac{1}{2} gt^{2} = (6.5 - \frac{9.81}{2}*0.663)*0.663 = 2.15$. I can confirm your calculations. In conclusion: The model is inapplicable to the code. It does not accurately represent what the code actually does. This is due to localplayer shenanigans; if you spawn an entity with an initial velocity of $(0, 6.5, 0)$ and an acceleration of $(0, -9.81, 0)$ it should indeed rise to $2.15 [m]$ before falling.

float jump_height = 1.1f; // TODO: better than a magic number in autojump code BTW...

[appgurueu]

Alright @SmallJoker, I have identified the shenanigans:

in client/clientenvironment.cpp:

		// Apply physics
		if (!free_move) {
			// Gravity
			v3f speed = lplayer->getSpeed();
			if (!is_climbing && !lplayer->in_liquid)
				speed.Y -= lplayer->movement_gravity *
					lplayer->physics_override_gravity * dtime_part * 2.0f;

			// Liquid floating / sinking
			if (!is_climbing && lplayer->in_liquid &&
					!lplayer->swimming_vertical &&
					!lplayer->swimming_pitch)
				speed.Y -= lplayer->movement_liquid_sink * dtime_part * 2.0f;

		}

some dingus apparently thought it would be fun to just double the gravity (presumably because )

Experiments show this is indeed the case: The acceleration must be doubled, halving the time and thus also decreasing the jump height!

This also means I have missed fixing this for local players, having incorrectly assumed they would rely on collisionMoveSimple too.

[appgurueu]

Substituting twice the gravity yields an ideal jump height of 1.0767074413863 - just above one block.

The previous jump height seems to have been slightly below one block serverside due to the error; clientside it would've been pretty much one block. This didn't matter too much due to stepheight though.

[appgurueu]

Bump. Opinions are needed on this. I now believe that this should actually be considered a crucial bugfix; the "breakage" (minor deviations in jump height) seems acceptable to me.

[benrob0329]

So with some playtesting, I don't see this breaking anything. It feels ever so slightly more "floaty", but nothing drastic and nothing I'd consider bad. I played both Inside The Box's "Hopscotch" (one of the oldest parkour levels on there), along with A.E.S.'s Tower Of The Gods, and both seem fine.

The only thing I will note is that there seems to be a bug with the old movement code that's used on AES, where you'll sink down about half a block when standing off an edge, see below:

[sfan5]

Testing this in singleplayer I can barely feel a difference, if anything I could agree with the "more floaty" @benrob0329 mentioned.
So I guess this is fine for the usual Minecraft-y games usecase.

What I'm worried about is the following:

• There is no way to get consistent movement between players of different versions (okay there's old_move but you don't want to import all the disadvantages)
• What about games that use more extreme acceleration values? They will be more affected, and this also applies to entities.

[sfan5]

So what about my concern? Anyone want to chime in?
I am not opposed to merging this but would like to avoid a situation where this change blows up at us once 5.7 is out and the first servers have it.

[appgurueu]

There is no way to get consistent movement between players of different versions (okay there's old_move but you don't want to import all the disadvantages)

There never was consistent movement to begin with; players with higher FPS fall slower currently. This PR makes all players equal (the same would happen if FPS approached infinity).

What about games that use more extreme acceleration values? They will be more affected, and this also applies to entities.

I don't think games can successfully use extreme acceleration with the current buggy behavior. If they did, they must have relied on certain fixed stepsizes for consistent behavior.

[SmallJoker]

Works. No differences found in the old movement code, new movement code and the autojump feature.

[Wuzzy2]

Wait, how did you find "no differences" if the author admits it's a breaking change and there were even graphs that show differences?

I'd be surprised if really no parcours map will be broken by this. There are many such maps, the probability that ALL of them still work perfectly seems low. This could seriously blow up. You're basically gambling here, and praying that nobody will notice; it does not seem it was even remotely tested enough yet.

What's worse, when there IS a parcours map that breaks, I'm afraid there will be NO workaround for this, other than releasing two different versions for this map.

This could probably one of biggest breaking changes since 5.0.0, and you brush it all off. I'm honestly shocked that such a risky change is seriously considered for a minor version, rather than a major version. Especially with the lack of testing, lack of input from players or server operators.

I strongly suggest to move this to the 6.0.0 milestone instead; better safe than sorry.

[Wuzzy2]

Compromise solution: Put this change in anyways and do the next release like you would usually do, except you call it 6.0.0. This would at least communicate the risk clearly.

[appgurueu]

Compromise solution: Put this change in anyways and do the next release like you would usually do, except you call it 6.0.0. This would at least communicate the risk clearly.

I apologize for initially misassessing the potential breakage. Further analysis has led to the conclusion that this is not a breaking change but rather a bugfix; this could even go into 5.6.1 IMO.

If the next version was to be a major one, many other breaking changes would be introduced as well to properly use the version bump.

[SmallJoker]

@Wuzzy2 I would like to highlight that the focus should be put on server-side entity movement. As mentioned above, little to no difference on client-side is totally expected due to the small time increments and thus more accurate calculation. The server however uses a 90 ms tick, which results in increased jump heights (see #12353 (comment) ) for mobs.

EDIT: To be fair, I did not find a mod where the jump height could be tested against side-effects. If you have a suggestion, please let me know.