[3.x] Physics Interpolation - Add InterpolatedProperty#104920
[3.x] Physics Interpolation - Add InterpolatedProperty#104920lawnjelly merged 1 commit intogodotengine:3.xfrom
InterpolatedProperty#104920Conversation
83f3ac7 to
80d8496
Compare
| first_print = false; \ | ||
| } \ | ||
| } else \ | ||
| ((void)0) |
There was a problem hiding this comment.
Note: I just changed this to be slightly more efficient in the case of slow checks.
It now checks the static before doing the condition, so once it has triggered the first time, it no longer does the condition check.
rburing
left a comment
There was a problem hiding this comment.
This looks quite nice from the API perspective, and I appreciate the defensive dev checks.
I'll have to defer to you for the implementation. I can say it looks believable.
| T prev; | ||
|
|
||
| public: | ||
| void pump() { |
There was a problem hiding this comment.
I'm not familiar with the term "pump" in this context. Is it a common name for this operation?
How about adding a comment? Or maybe a more descriptive name like snap_to_current_value / jump_to_current_value?
There was a problem hiding this comment.
Naming is always one of the most difficult problems in programming 😁 .. I've tried a few different names for this over the years but currently am tending towards pump or fti_pump. It essentially acts like a pump, on every tick you are moving current values over to previous values (it is also sometimes reused for reset, but resetting refers to the intention in a specific circumstance, not the mechanism).
(This is also why teleport is imo a better naming than reset_physics_interpolation, because it makes the intention clear, whereas reset is very vague and users often get confused about it.)
This pump is the core of how fixed timestep interpolation works, it relies on maintaining a continuous flow of previous and current transforms / properties / whatever, and then interpolating between them using the interpolation fraction.
I'm happy to put some more comments in.
snap_to_current_value / jump_to_current_value I'm not sure describe what is going on, I for one have no idea what they are referring to (and I've been using FTI for 25 years lol 😁 ).
There was a problem hiding this comment.
Actually thinking about it, it may be possible to use fti_tick() here, let me have a look through.
Hmm, not absolutely sure tick is an improvement, as fti_pump() is also used for resets, and not just ticking, so in exchange for a more familiar name we would be introducing an element of confusion. What happens if we start requiring users / contributors to call tick() outside of ticks?
I'm tending towards preferring sticking with bespoke terminology for physics interpolation, because in retrospect I think Juan fell into the same trap here with this desire to use "familiar" words:
- "FTI" became "physics interpolation" - caused confusion because it is nothing to do with physics
- "teleport" became "reset physics interpolation" - again users have no idea what this does or when to use
And add some basic interpolated properties to Camera.
80d8496 to
18c01b2
Compare
|
Have pushed some changes in response to the suggestions:
|
|
Thanks! |
Adds framework for
InterpolatedProperties.Adds interpolated properties to
Camera.Interpolates:
Implements godotengine/godot-proposals#9964 .
Notes
Cameraprovides a good test case for adding some custom interpolated properties.InterpolatedPropertyemulates the underlying type (for when FTI is off), but has some extra functionality available.fti_pump()for more reset functionality inSpatialderived classes.reset_physics_interpolation()user calls.Discussion
While implementing this I made some minor adjustments to
SceneTreeFTIto better support properties.Properties aren't inherited and concatenated like scene tree xforms, so it turned out these are more efficient to handle separately.
SceneTree, as often only a few nodes will need properties interpolating.Spatialbase class, so in most custom cases in the future contributors will be notifying custom property changes.fti_pumpandfti_update_serversOne slightly controversial change it that
fti_pump()andfti_update_serversis now separated into two versions - for xforms and for properties.As far as I can see this is the most efficient way of handling it, even if it may look slightly more confusing in say the
Cameraclass, where we have to update servers for both xform and properties.An alternative would be to send e.g. a
boolwith the two routines, however there is more opportunity to "shoot yourself in the foot" with that approach, particularly regarding calling the parent class routines.In practice, most classes that custom interpolate will only be updating properties, and will rely on
Spatialfor xforms, so will only have to implementfti_pump_property()andfti_update_servers_property()-Camerais to some extent an anomaly and a worst case situation, so good to handle first.