-
-
Notifications
You must be signed in to change notification settings - Fork 21.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add move_toward_smooth
helper
#92236
base: master
Are you sure you want to change the base?
Conversation
Please open a proposal to track the support and details of this feature |
|
1daf722
to
a129ecb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good generally, save for the documentation details and the addition of a dedicated method to the engine, especially to handle the single/double precision behavior
a129ecb
to
41fc1d8
Compare
Sorry, what do you mean by "save for"; is it a typo for "[it is] safe for"? |
I mean "except for", an English phrase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise this looks good I believe, will need general approval and decision on the necessity, but looks good code wise
Would be appreciated if you also added tests for expm1
but that's not necessary and can be added later
Edit: We can also consider binding expm1
to scripting down the line as well, but out of scope for this IMO
1f25712
to
fefec26
Compare
Sure, added |
Also, forgot, we might want to add this to the C# glue as well, what do you think @godotengine/dotnet? Can be done in a follow-up unless they think it's relevant to add it there too But no need to add that right now until we get a full go-ahead from the core team on the need for this |
This will be handy in many demo projects as I keep forgetting how to do smooth lerping in a FPS-independent manner. |
I don't mind doing the change but testing it is going to be challenging for me On an unrelated note, I've been thinking more about the API. Instead of Benefits:
Drawbacks:
Thoughts? |
This is definitely a good to have core feature. Would it make sense to add SLERP versions?
please not that, worst of all options Opinion: Delta at the end feels more natural as the other 3 args are user defined whereas delta is unknown: move_toward_smooth(from, to, rate, delta) |
I'm not familiar enough with the involved maths to provide an educated answer
Ok so we have 3 choices for
Is it possible to have Godot core team take on the API? |
fefec26
to
8852f91
Compare
Since there was no answer I made my pick and pushed a new version. Changes from last time:
|
how many slerp/lerp functions exist? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, this doesn't appear to be independent of FPS (or physics tick rate for things in _physics_process()
).
I've changed the Voxel demo to make use of move_toward_smooth()
for FPS-independent smoothing. The project below uses 120 physics ticks per second; if you change it to 30 in the project settings, notice how player movement acceleration becomes much slower (while it should remain just as fast). Try creating a flat world in the project so you can test this more easily.
The player movement code is in player/player.gd
. You can check the original demo here: https://github.com/godotengine/godot-demo-projects/tree/master/3d/voxel
Testing project: voxel_move_toward_smooth.zip
I haven't checked the test project in detail, but just to point out, this code is purely math, it doesn't touch physics, and should therefore not be tested with a project that uses physics, because that would be conflating two different things. The only relevant factor is physics tick rate. A suitable test would therefore be something like moving a box across the screen at different tick rates. This is basic to the scientific method - remove any extraneous variables. |
The code is:
You need to use |
Right, I had assumed it was automatic for some reason. |
Great work, this is both simple to use and immensely useful! This implementation is far more elegant than Unity's Just a couple of notes:
|
c486dd0
to
8cff7a0
Compare
Shouldn't this remain an undefined or unspecified behavior? Delta is not really supposed to be something else than the
I added "Note that due to the nature of the exponential function, the return value will quickly get close to the target [param to] but may never reach it. It is recommended to rely on [method is_equal_approx] to test whether the target is reached." BTW, I did have to make 7 variants of that same documentation (1 gds function, 2 gds vector methods, 2 c# functions, 2 c# vector methods); this is kind of a maintenance burden, I don't know if extending the documentation extensively makes a lot of sense in that situation. |
Agreed, and personally I can't think of any good use for a negative value. Though the documentation for the original
The difference should be immediately obvious though (using the "smooth" version will move away much faster), so I don't think it's really necessary to document this aspect. The other point was much more important, thanks for adding the note. And yeah, I agree that documenting 7 variants of what is essentially a single function is kind of a burden, but there's no way around that as far as I'm aware of. Speaking of this, once these changes are merged, the example in the tutorial about interpolation (section "Smoothing motion") should eventually be changed, |
8cff7a0
to
8f8fb4f
Compare
You can't really tell the user what to do with this function, I suppose.
Yyyyyeah. We can't easily maintain all of them in quality, but at least all of them should mention accurate information. |
In that case, is it OK to document it as undefined behavior? Right now the negative delta doesn't seem useful or meaningful and we may find a more relevant behavior in the future. Documenting a behavior with a negative value implies that it's a legit case (here, as far as I know, it's not). Also, this PR is opened since 7 months for something fairly small: what really is blocking here? |
Yeah.
Core review approval. It's new, exposed API and regardless of size it's likely worth evaluating whether this function is worth having in core. |
not implemented for all the things that |
It's not really meant to be a replacement for This function, In Unity, this also seems to be the general philosophy. |
@danijmn Thank you, I indeed aligned
I don't know if I misunderstood something here, but I implemented it for vec2 and vec3, in C# as well |
Sorry, bad choice of words! I amended the comment. |
If we go purely by the existing API methods shouldn't this PR also include On a sidenote to me as a user it doesn't really make sense that there's |
You're right, if we go down the route of matching the existing API, we should also cover
Some tests:
Adding variants for all other kinds of IMO adding variants for every type covered by |
Quick clarification because I am partially at fault.
Working on that! These are all analogous for |
I didn't test if the documentation in the XML files were rendered correctly because I couldn't find a way to preview it locally; I'd be happy to check it if someone can provide the informationEdit: looked it up from within the GUI instead of looking for a local HTML exportI'm not sure if I need to document something in the Changelogapparently this is done separatelytime
constant (equal to1/rate
) instead ofrate
, but I feel like it can be more confusing when there is alreadydelta
in the parameters which also a "time unit"