RotateOverTimeModifier - #552
Conversation
djeedai
left a comment
djeedai
left a comment
There was a problem hiding this comment.
I'm skeptical about this change. See individual comments. As is I'm not convinced.
| pub struct RotateOverTimeModifier { | ||
| /// Rotation that the particle will have in a second. | ||
| /// | ||
| /// Expr type: Mat4 |
There was a problem hiding this comment.
I don't think we want that Mat4 field, users are already struggling with vector operations and complaining there's too many missing helpers, so for matrices where we have almost nothing, basically nobody will use this modifier. Not your fault, but basically I don't want to try encouraging users to use Mat4 and drive them into a corner where there's no utilities to manipulate matrices; that will just lead to more opened issues and frustration.
There was a problem hiding this comment.
Replaced it for a Vec3 with euler angles, also have the advantage of being able to represent rotations of over 360 degrees
| context.main_code += &format!( | ||
| r#" {{ | ||
| let rotation = {rotation}; | ||
| particle.{0} = normalize(mix(particle.{0}, (vec4(particle.{0}, 1) * rotation).xyz, {dt})); |
There was a problem hiding this comment.
For me RotateOverTimeModifier (and this is confirmed by its doc comment) means that the particle will continuously rotate. This is not the case; this modifier interpolates the rotation of the particle from its current rotation to a target one, then does nothing after that.
The other major issue is that calling mix(a, b, dt) only works nicely if dt < 1, otherwise the interpolation will overshoot. This is because we're mixing units; dt is in seconds here, whereas the mix(.., .., r) ratio is a unitless quantity in 0..=1. If you really intend to create an interpolator modifier, it should take a duration parameter that determines how long it should take to go from current to target rotation, and then calculate the unitless mix(.., .., r) ratio such that it takes that much time relative to dt.
There was a problem hiding this comment.
The matrix indicates the rotation that the particle will do every 1 second, dt will always be way below 1 second unless there is immense lag going on, so the rotation will accumulate over the lifetime of the particle
| name = "lightning" | ||
|
|
||
| [[example]] | ||
| name = "rotate_over_time" |
There was a problem hiding this comment.
In general please avoid adding examples unless there's a non-obvious new feature or there's really no other example this could fit in. The reason is that the maintenance cost of examples is quite high; not only are examples used as behavior benchmark to validate behavior for all major changes (need to run them all one by one for visual check), they also require extra work in wasm (add new webpage, extra compile time, etc.). Ideally all of that should be automated or covered by unit/feature tests, but until then it's quite the burden. Thanks!
There was a problem hiding this comment.
which example would it be ok to include this modifier then? if any?
2 participants
Very naive implementation of
RotateOverTimeModifier