-
-
Notifications
You must be signed in to change notification settings - Fork 224
delta
parameter in process()
+ physics_process()
is now f32
instead of f64
#703
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
Conversation
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-703 |
Isn't it equally confusing to start using My understanding was that Godot tries to use Of course, this convention can be a bit inconvenient when "space meets time" like in |
Is this a distinction that matters in practical scenarios? (I'm not even sure it generally holds). Looking at real impact, I observe the following:
So now that people for whom precision matters have a way to still access I need to balance the needs of different users, and believe this is a good compromise, especially since usage gets easier for the majority. Maybe I'm missing something and then we can re-evaluate, but it removes one known ergonomic issue right now 😉 |
It just feels weird that we now require a cast in a "time meets time" case instead, because we have one (?) instance of a time that is a Certainly, such use cases are more rare. They will mainly arise when working with audio, timer, and tweens. Last time I checked, they used But I guess what I'm really wondering about is this: The change seems to introduce a general disparity between GDScript classes and gdext classes: Let's say both classes accumulate the But I guess I'm pretty alone on this so optimizing for the majority use case is fine, and I can work around it on my end. |
These are good points indeed. I think you briefly mentioned this in #510:
But this sounded very vague and not like a workflow you'd actually use -- so, has that changed? Also, you stated that an extra line wouldn't bother you much, which would be the same situation after this change? 😉 Maybe in the end one of the more "magic" approaches is better if we want to support both use cases equally. Or we support My problem is that I don't have the data points on how many people actually use the |
@Bromeon Is it possible to add documentation for generated
|
@Bromeon Will this be a breaking change? For existing |
@coder137 Yes. It would technically be possible to add a deprecation magic within the proc-macro; I'd need to see how much effort it would be. Out of curiosity, how do you currently use |
@Bromeon I don't use process/physics_process a lot so refactoring should be fine even if it is a breaking change. Mostly in cases of movement/rotation (where I do Since delta is always small, in this case |
The reasoning is in the first message and the issue being closed by this PR; I'd like to not reiterate it 😉
The point here is that the cast is not only unnecessary for a majority of cases, but it's provably confusing (see the various people who stumbled upon this obstacle on Discord). What matters most to me is user experience and efficient workflows. More than "idiomatic Rust" for example. See also Philosophy. |
@bluenote10 This is not true btw, the Godot API is quite inconsistent when it comes to representation of times. Just some random samples: So even within the same domain, Godot sometimes uses different types. My conclusion here is:
Now that doesn't mean changing process parameters to |
Indeed that looks rather messy, thanks for collecting some examples!
I've been thinking about this some more as well. In principle that is what GDScript does as well when multiplying scalar floats (which are What would be the main argument against that solution? That a repeated cast has an unnecessary performance overhead? From a practical perspective I guess the probability of observing any measurable effect of casting a scalar several times instead of just once is pretty low. Perhaps it is sufficient to document on Does it justify a feature flag? Normally feature flags are opt-in, but here we would have a case of opt-out for the better default, so I'm not sure not if a feature like |
Nothing per se, but it can be a bit confusing since Rust's scalar types don't allow it. So you can write f64 * Vector3 * f32 but not f64 * f32 * Vector3 Well possible that it's not a big issue in practice 🤔
I don't think so, it's a very niche thing. |
Tried a bit more. Multiplying isn't really the solution, there are many other ways how I will leave this for now but will likely change it in a few weeks to Another option is having 2 methods |
I'll close this for now, needs more thought. Might reopen here or in another one. |
For virtual functions
Node::process()
andNode::physics_process()
, this changes parameterdelta
's type fromf64
tof32
in single-precision builds. In double-precision, they stayf64
. Effectively, the type becomesreal
.Reasons:
pos += vel * delta
currently needs a cast. Most code doesn't benefit from the extra precision.Node::get_[physics_]process_delta_time()
, which keeps returningf64
.Closes #510. See there for extended discussion.