Fix negative delta arguments

[zmanuel]

Three attack points, all after the regular calculations:

1. Prevent negative physics timestep counts. They could occur if
   physics_jtter_fix is changed at runtime.
2. idle_step is not allowed to go below one tick. That could
   happen on physics_jitter_fix changes or heavily fluctuating
   performance.
3. Prevent that the idle_step modification breaks the promise
   that Engine.get_physics_interpolation_fraction() is between
   0 and 1 by doing more physics steps than the base system wants.

Fixes #26887

[Faless]

Probably also fixes: #25166

[zmanuel]

I removed the positive limit on output deltas and only guarantee they are non-negative now. The PR is updated with the squashed result. The test project (#26887 (comment)) then complains about lots of zero deltas, of course, but no negative ones.

The previous code is still here: 7d29221

[nathanfranke]

Why would there be zero delta? Is the frame rate higher than 100,000?

[zmanuel]

No, but the incoming delta times fluctuate by much more than the average delta and also by more than 1/physics_fps. The system here tries to keep the outgoing deltas stable within the given bounds by physics_jitter_fix, and that sometimes means the "game clock" (sum of outgoing deltas) is ahead of the real clock (sum of incoming deltas), this will happen if the framerate increased from one moment to the next. And then again sometimes, the system decides that being ahead was a mistake and that it should resync game and real clock while the current incoming delta is very small, smaller than the difference between the two times. The bug was that it would do so by letting the game clock snap back, creating a negative delta. Now it just stops for a bit.
The previous revision had it slow down instead, which was a bit iffy because it involved two magic numbers, the absolute minimum delta (for your >1E+6 FPS case, for example) and the slowdown factor.

Dismissing review

[zmanuel]

So, discussion here is a biiiit slow and it would be nice to get this fix in, because the current situation of negative delta is just no good. I've amended the fix here:
https://github.com/zmanuel/godot/tree/negative-delta-fix-unsquashed2
Two changes:

• the "in case we need it" debug code is removed; that has caused some confusion and is of little value
• the minimal timestep is now the measured timestep divided by 8. Not 0, that's apparently not universally loved. It's still a hardcoded number, but pretty much all values between 2 and 1024 have the same effect; it's not a magic value that needs fine tuning. So I'm personally OK with it.

It's also rebased to current master. As the name says, the changes are not squashed to a single commit yet.

What's the procedure there? Obviously, squash. But then, do I just force push it to update this MR? Or do I make a new MR, refering back here? Ordinarily, I'd update, but between the original submission and now was the call to resubmit all MRs anyway. That call also said old MRs would be deactivated/rejected/closed, which hasn't happened here.

[akien-mga]

You can force push to update this PR, the announcement we made that old PRs might be closed has not really been enforced, we're doing our best to try to integrate whatever was submitted in the last and keep being updated by their author.

[jordo]

So, discussion here is a biiiit slow and it would be nice to get this fix in, because the current situation of negative delta is just no good.

I'm hoping to get some of the reviewers eyes on this soon.

[lawnjelly]

We discussed this PR a bit today in rocket chat:
https://chat.godotengine.org/channel/devel/thread/ECRSuitLBW9BxSiTJ

and the general consensus was at least me and reduz were happy to try this out in the 3.4 betas. That is assuming it has been tested and appears to work as well as can be tested prior to a wider rollout. If there are any problems I'm guessing they will appear pretty soon.

In the long run we might do some improvements to the timing code / refactoring but this should be good to fix the bugs in the current version,.

[jordo]

We discussed this PR a bit today in rocket chat:
https://chat.godotengine.org/channel/devel/thread/ECRSuitLBW9BxSiTJ

and the general consensus was at least me and reduz were happy to try this out in the 3.4 betas. That is assuming it has been tested and appears to work as well as can be tested prior to a wider rollout. If there are any problems I'm guessing they will appear pretty soon.

In the long run we might do some improvements to the timing code / refactoring but this should be good to fix the bugs in the current version,.

thx @lawnjelly ! Sounds like a good appoach

[lawnjelly]

@zmanuel sorry there's been a bit of a delay on this, I think if you are able to rebase to fix that small double / float change this should be fine to merge and try out.

[zmanuel]

@zmanuel sorry there's been a bit of a delay on this, I think if you are able to rebase to fix that small double / float change this should be fine to merge and try out.

Sure, done. And no worries.

Also, feel free to completely remove the physics_jitter_fix-code once your frame time smoothing solution is complete (maybe it already is? I admit I haven't been paying close attention, I just saw it in the release notes). This here has no significant practical advantages; it converges quicker to stable updates, that's about it. IMHO, that does not justify the really hard to follow logic with the typical_physics_steps.

[Calinou]

once your frame time smoothing solution is complete (maybe it already is? I admit I haven't been paying close attention, I just saw it in the release notes).

It's not available yet; it's planned for a future 4.x release.

[lawnjelly]

once your frame time smoothing solution is complete (maybe it already is? I admit I haven't been paying close attention, I just saw it in the release notes).

It's not available yet; it's planned for a future 4.x release.

I think this is referring to #48390 rather than the timestep interpolation, me and @zmanuel discussed it on rocketchat a while ago.

While they may both solve the stairstep effect in some circumstances, I'm happy to get this PR in, and we can look at a later date at whether there is an argument to remove jitter fix.

At the moment the delta smoothing operates only in limited circumstances (vsynced, and when meeting vsync intervals), and jitter fix may still be helping in wider circumstances. We probably need to test this in more detail before deciding whether there is a route to moving away from jitter fix.

[lawnjelly]

The PR isn't showing as mergeable as it still thinks there are requested changes. @Calinou is there still something that needs to be done or is this a github hiccup? 🤔

[zmanuel]

The PR isn't showing as mergeable as it still thinks there are requested changes. @Calinou is there still something that needs to be done or is this a github hiccup? thinking

There were some conversation items relating to outdated code changes that were considered unresolved; apparently I was able to mark them as resolved just now. Maybe those were the hangup.

At the moment the delta smoothing operates only in limited circumstances (vsynced, and when meeting vsync intervals), and jitter fix may still be helping in wider circumstances.

I doubt jitter fix does anything useful for non-vsync, the traditional one with tearing. But yeah, it may stabilize timesteps with variable sync (G-Sync/FreeSync) if the framerate is semi-stable. But in either of those cases, physics jitter would not have been a problem anyway, and I see no reason one could not teach the delta smoothing to work in a limited fashion in those situations, too.

[lawnjelly]

It looks like everyone interested has had a chance to look this over (Akien, reduz etc) and it looks good to me for testing out in master imo, worst case if we get problems it can be reverted.

Thanks!

[akien-mga]

This would need a dedicated backport PR for 3.x as the code has changed significantly (mostly parameter renames, but I prefer not to mess it up by eyeballing the changes while cherry-picking several PRs).

[zmanuel]

This would need a dedicated backport PR for 3.x as the code has changed significantly (mostly parameter renames, but I prefer not to mess it up by eyeballing the changes while cherry-picking several PRs).

Sure thing, here you go: #52947
I had an old version of the fix that still used the 3.x variable names around, that way I had something to cross-check.