Add optional delta parameter to move_and_slide#84665
Add optional delta parameter to move_and_slide#84665monitz87 wants to merge 10 commits intogodotengine:masterfrom
Conversation
This overrides the implicit delta derived from context that is currently being used. If not passed, the method functions as it normally does. This change allows finer control over the method's behavior without sacrificing ease of use.
|
Thank you for your contribution! You need to add the new parameter to the documentation, use |
Done. My bad for missing that part 😅 |
|
What happens when |
|
Fair enough. Should I just make the default 0.0? |
|
I think |
|
Should probably document that passing non-positive delta leads to unexpected behavior |
|
That it uses the default one yes, that it ignores it, not necessarily unexpected behaviour, should be clear what it does |
Also add a note to the docs about the behavior when delta is <= 0
|
Might want to change the check to Will do more style and detail review later but will leave for testing for right now, can test and investigate when I have more time |
Why is that? In the current docs I see examples of both |
|
Unsure, it's stripped by the generator I think, which is an issue, can't find it right now though Edit: might have updated since then, had it backwards in any case |
Also fix documentation of delta param
|
The current thing that's failing is compatibility related, a bit of a complex thing to add so no rush with that right now, will guide you through it when reviewers have gone through the feature itself |
|
This doesn't sound sufficient on its own to perform rollback. If you're catching up with server lag, performing |
Thanks :D |
It's not sufficient on its own, but it's still a necessary step to be able to use
Technically, this is a separate issue from being able to step the physics engine, even if they're related to the same use case, so it made sense to me to raise a separate PR for it.
PS: just to be clear, the comment this PR refers to doesn't allude to performing move_and_slide once with a really large delta. It refers to performing it once per physics frame with a fixed delta, but having the engine tick faster, so that the client actually simulates the game faster and sends inputs to the server at a higher frequency when the server signals that it is being starved |
With this PR, multiple simulation steps for character movement could be easily implemented by just calling move_and_slide() multiple times in a for loop, with smaller delta, no? |
|
@monitz87 I thank you for this contribution, glad to see this feature is finally getting added after the over 2 years of discussion about it in issues! Been banging my head against a wall trying to figure out why I was getting constant desyncing in my netcode to find that move_and_slide was the culprit. |
|
I just stumbled accross this problem myself. What is holding this back from being merged? |
nongvantinh
left a comment
There was a problem hiding this comment.
Please squash it into a single commit and resolve the conflict with the master branch.
scene/2d/physics_body_2d.cpp
Outdated
| if (!(delta > 0)) { | ||
| // Hack in order to work with calling from _process as well as from _physics_process; calling from thread is risky. | ||
| delta = Engine::get_singleton()->is_in_physics_frame() ? get_physics_process_delta_time() : get_process_delta_time(); | ||
| } |
There was a problem hiding this comment.
| if (!(delta > 0)) { | |
| // Hack in order to work with calling from _process as well as from _physics_process; calling from thread is risky. | |
| delta = Engine::get_singleton()->is_in_physics_frame() ? get_physics_process_delta_time() : get_process_delta_time(); | |
| } | |
| if (delta <= 0) { | |
| // Hack in order to work with calling from _process as well as from _physics_process; calling from thread is risky. | |
| delta = Engine::get_singleton()->is_in_physics_frame() ? get_physics_process_delta_time() : get_process_delta_time(); | |
| } | |
There was a problem hiding this comment.
This is incorrect, see the comment above, this handles invalid values such as NaN
There was a problem hiding this comment.
Interesting, I have never seen this way to catch NaN used before. The expression is really confusing, though it's valid.
There was a problem hiding this comment.
@AThousandShips Shouldn't we use Math::is_finite(delta) to be more expressive?
There was a problem hiding this comment.
This is an established syntax used in several places, the comment is clear enough and there's no need to make a less efficient check IMO
There was a problem hiding this comment.
In my opinion, since the user wants to pass in their own delta, they are supposed to provide a valid value. If the provided value is invalid, the engine should report the error and stop executing the remaining code instead of enforcing a random choice from the engine to avoid unexpected surprises.
There was a problem hiding this comment.
This would be noisy and there's no random choice here it is perfectly reasonable IMO, but that should be discussed in the proposal instead as it's a design derail, not an implementation one
scene/3d/physics_body_3d.cpp
Outdated
| if (!(delta > 0)) { | ||
| // Hack in order to work with calling from _process as well as from _physics_process; calling from thread is risky. | ||
| delta = Engine::get_singleton()->is_in_physics_frame() ? get_physics_process_delta_time() : get_process_delta_time(); | ||
| } |
There was a problem hiding this comment.
| if (!(delta > 0)) { | |
| // Hack in order to work with calling from _process as well as from _physics_process; calling from thread is risky. | |
| delta = Engine::get_singleton()->is_in_physics_frame() ? get_physics_process_delta_time() : get_process_delta_time(); | |
| } | |
| if (delta <= 0) { | |
| // Hack in order to work with calling from _process as well as from _physics_process; calling from thread is risky. | |
| delta = Engine::get_singleton()->is_in_physics_frame() ? get_physics_process_delta_time() : get_process_delta_time(); | |
| } |
…nd-slide-delta
|
@monitz87 @AThousandShips I don't know if this PR is still being worked on but I'd like to push it across the finish line. I've added the missing compatibility functions which were causing the tests to fail but I'm not sure if I should create a new PR or push the changes onto this one. |
|
@gordongu no complaints from me if you want to take over this PR |
This overrides the implicit delta derived from context that is currently being used. If not passed, the method functions as it normally does. This change allows finer control over the method's behavior without sacrificing ease of use.
Explanation for why this is necessary here
This is my first contribution to the project, so I apologize in advance if I botched something. I read the contribution guidelines but there's a chance I missed something