Animate Bindings Reactivity #10387
Animate Bindings Reactivity #10387
Conversation
| self.animation_data.borrow_mut().reset(); | ||
| let mut animation_data = self.animation_data.borrow_mut(); | ||
| animation_data.reset(); | ||
| if let Some((details, start_time)) = (self.compute_animation_details)() { |
There was a problem hiding this comment.
I think it might not be good to call something that can query other properties from mark_dirty or we could end up in bad recursion.
Was it a problem to query it on the next frame in ShouldStart as before?
There was a problem hiding this comment.
Okay, I wanted to discuss this anyhow. This is a quick fix to be able to use current_tick() for an initial test and definitely not optimal. We'll probably want to modify the Animation::Transition type so that we can make the time Option.
Another downside to evaluating the PropertyAnimation inside evaluate is that from a user perspective it becomes very hard to predict when the animation data is computed.
It doesn't happen when the binding to the animated property changes, but rather when the animated property is first read, which could be unintuitive.
@NigelBreslaw agress that the values should remain constant after the animation starts ( #348 (comment) ), but this makes it somewhat hard to determine when exactly "the animation starts".
But maybe documenting this behavior of "the animation data is queried when the property is first read after modification", is probably good enough.
LeonMatthes
force-pushed
the
lm/348-animate-reactive
branch
from
23bd875 to
969095e
Compare
This avoids re-evaluating the PropertyAnimation during the mark_dirty function and re-evaluates it on the first call to evaluate.
This has been replaced by set_animated_binding_for_transition
The `state` variable is referenced by both expressions, so using a single CodeBlock is easier. In the generated code, use tuple destructuring to convert the Instant to an Option<Instant>.
LeonMatthes
force-pushed
the
lm/348-animate-reactive
branch
from
969095e to
1bd8318
Compare
ogoffart
left a comment
ogoffart
left a comment
There was a problem hiding this comment.
Great, thank you.
Please squash and merge
| &animation_data, nullptr); | ||
| } | ||
|
|
||
| template<typename F, typename Trans> |
There was a problem hiding this comment.
Style nitpick: Maybe this can be renamed A for animation
(Out of scope for this PR, but concepts here would really help to read this code as to what this type is supposed to be)
* Add test case for reactive Animate bindings See issue slint-ui#348 * Add C++ test case for animate bindings reactivity * Fix animted property bindings for C++ and Rust * Fix animate bindings reactivity in interpreter * Use optional start time instead of current_tick This avoids re-evaluating the PropertyAnimation during the mark_dirty function and re-evaluates it on the first call to evaluate. * Remove set_animated_binding This has been replaced by set_animated_binding_for_transition * Rename set_animated_binding_for_transation to set_animated_binding * Revert Animation::Transition to a single CodeBlock The `state` variable is referenced by both expressions, so using a single CodeBlock is easier. In the generated code, use tuple destructuring to convert the Instant to an Option<Instant>. * Fix set_animated_binding unit tests
* Add test case for reactive Animate bindings See issue slint-ui#348 * Add C++ test case for animate bindings reactivity * Fix animted property bindings for C++ and Rust * Fix animate bindings reactivity in interpreter * Use optional start time instead of current_tick This avoids re-evaluating the PropertyAnimation during the mark_dirty function and re-evaluates it on the first call to evaluate. * Remove set_animated_binding This has been replaced by set_animated_binding_for_transition * Rename set_animated_binding_for_transation to set_animated_binding * Revert Animation::Transition to a single CodeBlock The `state` variable is referenced by both expressions, so using a single CodeBlock is easier. In the generated code, use tuple destructuring to convert the Instant to an Option<Instant>. * Fix set_animated_binding unit tests
2 participants
Allow animate properties with a binding to re-query their PropertyAnimation whenever the binding to the property changes.
Closes #348