FreeCam: change walk_speed scaling to avoid sticking to zero/negative speeds
#21486
Conversation
janis-bhm
added
C-Bug
|
It probably makes sense to have a math person take a look at this to make sure I'm not doing anything egregiously terrible here. |
alice-i-cecile
requested review from
IQuick143,
Jondolf,
NthTensor and
janhohenheim
| if amount != 0.0 { | ||
| // scale the speed exponentially with the scroll factor as the base. | ||
| let scroll_plus_one = controller.scroll_factor.max(0.0) + 1.0; | ||
| let log_speed_plus_one = bevy_math::ops::ln(controller.walk_speed + 1.0); |
There was a problem hiding this comment.
What's the logic behind doing ln(x + 1) here? This seems like a very unnatural choice of function.
There was a problem hiding this comment.
my thought process was that I want the speed to represent a^x - 1 where a is the scroll factor, and adjust the speed with the derivative at each scroll step.
So ln(speed+1) is roughly the inverse of that to get x from the speed,
scroll_factor starts at 0, but I need it to start at 1 to be the base
and a^x is (very) roughly the derivative of the function I started with.
Then subtract a value close to 1 to get the scaling factor below 1.
| let log_speed_plus_one = bevy_math::ops::ln(controller.walk_speed + 1.0); | ||
| // `factor` will approach but never reach 0 as `walk_speed` approaches 0: | ||
| // Importantly, we want to avoid ever reaching exactly 0 so we don't get stuck there. | ||
| let factor = bevy_math::ops::powf(scroll_plus_one, log_speed_plus_one) - 0.999; |
There was a problem hiding this comment.
(scroll + 1)^ln(speed + 1) is difficult to interpret. One could equally rewrite it as:
(speed + 1)^ln(scroll + 1), which is slightly easier to see that the walking speed is being... kinda raised to some power given by the scroll factor. But still, this doesn't seem to make any sense unit wise. The speed is some sort of velocity, in say, m/s. 1 has no unit, so it's weird we're adding it on there, and then we're raising it to the power ln(scroll + 1), giving the result a unit of (m/s)^ln(scroll + 1), which is really weird, what does this value mean?
| // `factor` will approach but never reach 0 as `walk_speed` approaches 0: | ||
| // Importantly, we want to avoid ever reaching exactly 0 so we don't get stuck there. | ||
| let factor = bevy_math::ops::powf(scroll_plus_one, log_speed_plus_one) - 0.999; | ||
| controller.walk_speed += factor * amount; |
There was a problem hiding this comment.
If the goal is to scale the walk speed with scrolling, I would suggest refraining from additions on it completely and directly scale the value:
| controller.walk_speed += factor * amount; | |
| let scale_factor = bevy_math::ops::exp(amount * controller.scroll_factor); | |
| controller.walk_speed *= scale_factor; |
alternatively one can do (if this kind of interpretation for scroll_factor is preferred, but it additionaly requires that it is strictly positive):
| controller.walk_speed += factor * amount; | |
| let scale_factor = bevy_math::ops::powf(controller.scroll_factor, amount); | |
| controller.walk_speed *= scale_factor; |
Importantly this achieves things like being independent of how the scroll amounts are split across frames, which I believe the current code doesn't do.
There was a problem hiding this comment.
xa^e looks much simpler so I think I'll switch it to that, but if you don't mind I would really appreciate if you could comment on my train of thought behind my original logic above.
| let factor = bevy_math::ops::powf(scroll_plus_one, log_speed_plus_one) - 0.999; | ||
| controller.walk_speed += factor * amount; | ||
| // avoid negative speeds. | ||
| controller.walk_speed = controller.walk_speed.clamp(0.0, f32::MAX); |
There was a problem hiding this comment.
I would suggest preventing a zero here by using some minimal value for the walk speed. Even if it's very small, like 0.000001 (I wouldn't suggest a smaller value than that because of floats).
| controller.walk_speed = controller.walk_speed.clamp(0.0, f32::MAX); | |
| controller.walk_speed = controller.walk_speed.clamp(MINIMUM_FREECAM_SPEED, f32::MAX); |
There was a problem hiding this comment.
I thought about this but I decided that being able to reach a speed of zero might be desired; sometimes, you might want to be sure that you cant change your position by accidentally pressing the wrong buttons. I'm open to have my opinion on this changed, though.
| controller.walk_speed += scroll * controller.scroll_factor * controller.walk_speed; | ||
| controller.run_speed = controller.walk_speed * 3.0; | ||
|
|
||
| if amount != 0.0 { |
There was a problem hiding this comment.
I think this if is kind of redundant, but that's a nitpick.
alice-i-cecile
added
S-Waiting-on-Author
|
I have a suggestion:
This function can be tuned to do whatever we want (like reaching 0 or otherwise), and we don't need to deal with inverse functions and framerate dependence and etc... My suggestion for that function would be like |
|
|
|
anyway I just realised this overlaps a lot with #21453 which I somehow missed. |
… for the free camera controller (#21538) # Objective This PR refactors the `FreeCam` controller to improve its modularity and flexibility. Happy to adjust based on feedback or suggestions! Fixes #21456 ## Solution * **Splits the `FreeCam` system into two components**: * `FreeCam`: Stores the **configuration** of the camera controller, including key bindings, movement speeds, and sensitivity. This struct is immutable during runtime, ensuring that the core settings stay consistent. * `FreeCamState`: Manages the **dynamic runtime state** of the camera, such as the current pitch, yaw, velocity, and speed multiplier. This component allows for real-time adjustments to camera behavior without altering the original settings. * **Improves the example for debugging and testing**: A simplified example (`free_cam_simple`) has been introduced, allowing for easy testing and showcasing of the new FreeCam controller system. * **Fixes the bug with translation speed**: As noted in #21483, the camera’s translation speed would "stick" after excessive scrolling. This bug was still present in the `free_cam_controller` example. Following the approach suggested in #21486, the bug has been resolved in the new `free_cam_simple` example (I did not yet verify the existing ones). * **Clarifies documentation**: Documentation has been updated to reflect the new structure, clearly describing the roles of both `FreeCam` (static settings) and `FreeCamState` (dynamic state). ## Testing * For testing, I added a simple example `free_cam_simple` as a POC (this can be removed later if no longer needed). * Run with: `cargo run --example free_cam_simple --features="free_cam"` --------- Co-authored-by: syszery <[email protected]> Co-authored-by: Alice Cecile <[email protected]>
… for the free camera controller (bevyengine#21538) # Objective This PR refactors the `FreeCam` controller to improve its modularity and flexibility. Happy to adjust based on feedback or suggestions! Fixes bevyengine#21456 ## Solution * **Splits the `FreeCam` system into two components**: * `FreeCam`: Stores the **configuration** of the camera controller, including key bindings, movement speeds, and sensitivity. This struct is immutable during runtime, ensuring that the core settings stay consistent. * `FreeCamState`: Manages the **dynamic runtime state** of the camera, such as the current pitch, yaw, velocity, and speed multiplier. This component allows for real-time adjustments to camera behavior without altering the original settings. * **Improves the example for debugging and testing**: A simplified example (`free_cam_simple`) has been introduced, allowing for easy testing and showcasing of the new FreeCam controller system. * **Fixes the bug with translation speed**: As noted in bevyengine#21483, the camera’s translation speed would "stick" after excessive scrolling. This bug was still present in the `free_cam_controller` example. Following the approach suggested in bevyengine#21486, the bug has been resolved in the new `free_cam_simple` example (I did not yet verify the existing ones). * **Clarifies documentation**: Documentation has been updated to reflect the new structure, clearly describing the roles of both `FreeCam` (static settings) and `FreeCamState` (dynamic state). ## Testing * For testing, I added a simple example `free_cam_simple` as a POC (this can be removed later if no longer needed). * Run with: `cargo run --example free_cam_simple --features="free_cam"` --------- Co-authored-by: syszery <[email protected]> Co-authored-by: Alice Cecile <[email protected]>
3 participants
Objective
Fixes #21483
Solution
I changed the way the
walk_speedscaling is calculated, including clamping to [0.0, Inf), and ensuring the scaling factor never reaches zero.I've changed the scaling function from the existing product of three values to use
powfandlnso that values forscroll_factorabove 1 have useful effects.Testing
I used #21477 to test this