-
Notifications
You must be signed in to change notification settings - Fork 108
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
Implement update_anim pass #539
Conversation
eea9ac3
to
89e2a17
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.
Any examples using animation just doesn't work in this PR...
We could also use a link to linebender/rfcs#7 in this PR's description
masonry/src/render_root.rs
Outdated
if root_state.request_anim { | ||
root_state.request_anim = false; | ||
self.root_lifecycle(LifeCycle::AnimFrame(elapsed_ns)); | ||
if root_state.needs_anim { |
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.
I wonder if this check should be inside run_update_anim_pass
. I can understand why it isn't with post_event_processing
, but my understanding is that that will be removed?
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.
I need to write down my thoughts on this (and correct the RFC); The short version is that update_anims doesn't really have the semantics of a "rewrite" pass. It's more of a special event pass.
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.
Re-reading your question again, I realize I misunderstood it. You're right that the check is a bit superfluous. The pass already does it once per widget anyway.
masonry/src/passes/update.rs
Outdated
state.item.needs_anim = false; | ||
state.item.request_anim = false; |
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.
It's not clear to me why this code is here.
Currently, this stops any animation after one frame, if it requests a new frame in the handler.
What is the expected behaviour if an animation wants to last more than one frame?
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.
Yeah, I think I rushed this PR too much.
New version, which I've tested with the variable clocks example. Everything works fine now. |
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.
Code looks good to me, I can't really test this on my ultra-old Notebook, which doesn't seem to support Vulkan properly. Though I get a panic in parley which seems weird to me (as unrelated to Vulkan I guess)? Anyhow this is not related to this PR as I also get this on main.
masonry/src/contexts.rs
Outdated
self.widget_state.needs_anim = true; | ||
self.widget_state.request_anim = true; |
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.
I guess we want to either rename the needs_...
variables to be more clear, or document the difference between request_...
and needs_...
well.
At a first glance, the naming suggests a little bit redundancy here, at least it wasn't clear to me what the difference exactly should be.
(But I don't think that this should be done in this PR, and it's probably better to be consistent with the rest of the naming).
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.
I guess we want to either rename the
needs_...
variables to be more clear, or document the difference betweenrequest_...
andneeds_...
well.
Right now the fields all have doc comments that follow this pattern:
- request_xxx: "This widget explicitly requested XXX"
- needs_xxx: "This widget or a descendant explicitly requested XXX"
Once we do a documentation pass I'll explicitly note the pattern as well, at least in WidgetState and ARCHITECTURE.md
. If you think there's other places we could cover, I'm interested. This is the sort of pattern where you want redundancy in documenting it.
Although note that it's strictly an internal naming convention that library users won't be exposed to, so it limits the places we're likely to document it in.
I do like those names. I think they're concise, which is valuable, and the "request/needs" difference feels to me like it's easy to remember once it's been explained.
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.
This still doesn't work as expected, unfortunately.
state.item.needs_anim = false; | ||
|
||
if state.item.request_anim { | ||
state.item.request_anim = false; |
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.
Maybe we should have a comment here that most passes reset their needs
and request
after the call to the widget method, but that it's valid and expected for request_anim
to be called in response to AnimFrame
.
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.
Done.
masonry/src/render_root.rs
Outdated
let mut root_state = self.widget_arena.get_state_mut(self.root.id()).item.clone(); | ||
self.post_event_processing(&mut root_state); | ||
|
||
self.last_anim = Some(now); |
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.
I don't think this will have the behaviour we want. Consider for example:
diff --git a/xilem/examples/variable_clock.rs b/xilem/examples/variable_clock.rs
index 8bead4d80..ea7dd599f 100644
--- a/xilem/examples/variable_clock.rs
+++ b/xilem/examples/variable_clock.rs
@@ -60,7 +60,7 @@ fn app_logic(data: &mut Clocks) -> impl WidgetView<Clocks> {
};
}
},
- |data: &mut Clocks, ()| data.now_utc = OffsetDateTime::now_utc(),
+ |data: &mut Clocks, ()| data.weight = (data.weight + 100.) % 1000.,
),
)
}
This will not animate smoothly, but will instead to jump to the full value, because when the animation frame occurs, the time will be the time since the last frame, rather than 0
as it is before this PR.
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.
Would you mind taking responsibility for this PR, then? It seems like this is more in your wheelhouse than mine, if you're currently working on frame pacing.
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.
Can do. I think this only needs a very small change to be ready to land.
be909ac
to
531ac4b
Compare
I'm going to do a small tweak on your changes, pushing now. |
I think this is ready to merge, it's on the critical path, and Daniel's concerns are essentially addressed. I think Phillip is still on vacation and isn't going to review this week. Either way, unless a blocker comes up, I'll merge this tomorrow. |
// Routing DisabledChanged has been moved to the update_disabled pass | ||
LifeCycle::DisabledChanged(_) => false, | ||
// Animations have been moved to the update_anim pass | ||
LifeCycle::AnimFrame(_) => false, |
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.
Is this unreachable!
?
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.
Not quite. A container widget could request anim. If it recurses the lifecycle function over children, then that line of code is reached.
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.
Hmm. Is it meaningful for it to be reached?
That is, should we put a debug_panic
here? Is it ever sensible to send a different lifecycle event to the one you received down the tree.
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.
I wouldn't bother. This code is going away anyway.
This is part of the Pass Specification RFC: linebender/rfcs#7