Skip to content
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

Pass mutable scene builder reference to paint #10

Merged
merged 2 commits into from
Nov 26, 2022
Merged

Conversation

raphlinus
Copy link
Contributor

Instead of the paint method returning a Rendered which was just a newtype around SceneFragment, pass in &mut SceneBuilder. Also store a scene fragment in each Pod, which is the storage for these builders.

This change has a number of consequences. Less boilerplate in the paint method, and also less allocation, as the storage can get reused. More importantly, though we don't have the logic for this yet, it sets the stage for retaining scene fragments when widgets don't change.

Instead of the `paint` method returning a `Rendered` which was just a
newtype around `SceneFragment`, pass in `&mut SceneBuilder`. Also store
a scene fragment in each `Pod`, which is the storage for these builders.

This change has a number of consequences. Less boilerplate in the paint
method, and also less allocation, as the storage can get reused. More
importantly, though we don't have the logic for this yet, it sets the
stage for retaining scene fragments when widgets don't change.
@xarvic
Copy link
Collaborator

xarvic commented Nov 25, 2022

Could we make the scene_builder argument a member of PaintCx like it was in druid?

And i am trying to understand how widgets render their children. The paint method of Pod has no scene_builder parameter and there is no method to retrieve the SceneFragment of the Pod. I would be in favor of adding the scene_builder parameter and let the Pod apply transform and write its SceneFragment into that of its parent.

@raphlinus
Copy link
Contributor Author

raphlinus commented Nov 25, 2022

I'm not sure moving it into PaintCx is a win. In Druid it's only ergonomic because the Cx derefs into a piet RenderContext. Also, in Druid that RenderContext is basically constant for the scope of the paint cycle, where in Xilem each builder is local to the widget's scene fragment.

I haven't implemented containers yet (soon), so it's not clear from the code. Pod will have a method to return a scene fragment reference, and the parent will do append() with that fragment (also apply a transform based on layout / scrolling / etc). The motivation for this is retaining the rendered scene fragments, so you only call into paint when they're invalidated. The idea is that append is cheap - currently it is a handful of memcpy operations, but shouldn't allocate in the steady case, and if the memcpy operations do turn out to be expensive, we could go to non-contiguous storage.

Add a public method to retrieve the scene fragment from a Pod.
@raphlinus
Copy link
Contributor Author

Hopefully that commit makes what I'm trying to do a little clearer.

@xarvic
Copy link
Collaborator

xarvic commented Nov 25, 2022

Yes, thanks :)

From what i understand painting a child widget will look like this most of the time:

child.paint(ctx);
builder.push_layer(..., Affine::translate(child.origin().to_vec2()), ...);
builder.append(child.fragment());
builder.pop_layer();

What i wanted to say is that is could be done by the Pod of child if its paint method receives the parent SceneBuilder. This was also my rationale for proposing to include the scene_builder argument into the PaintCx

@raphlinus
Copy link
Contributor Author

Ah, now I understand. I was confusing the paint method of Pod with the paint method on the Widget trait.

In the general case we will not need push_layer(), only when doing clips and blends. Also note that the affine transform has changed from stateful (PostScript/canvas-like) to stateless, so it needs to be given to the append method. Thus, though we could certainly add a convenience method to Pod to append its fragment, I'm not sure it helps that much.

@xarvic
Copy link
Collaborator

xarvic commented Nov 25, 2022

Ah! I over read the transform parameter in append. Now it makes sense. Thanks for the explanation :)

Copy link
Contributor

@dfrg dfrg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! I spent some time with it but wanted to give others a chance to offer approval.

@raphlinus raphlinus merged commit 44369fb into main Nov 26, 2022
@raphlinus raphlinus deleted the mut_fragment branch November 26, 2022 21:11
github-merge-queue bot pushed a commit that referenced this pull request Nov 11, 2024
New tracy image:


![image](https://github.com/user-attachments/assets/94e54c89-8159-4dd4-a521-4a7122f64375)

New log tracing file:
<details><summary>An overview of the new logs</summary>
<p>

```
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}: masonry::passes::update: RootWidget received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#8}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#8}:SizedBox{id=#7}: masonry::passes::update: SizedBox received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#8}:SizedBox{id=#7}:Flex{id=#6}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#8}:SizedBox{id=#7}:Flex{id=#6}:Flex{id=#3}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#8}:SizedBox{id=#7}:Flex{id=#6}:Flex{id=#3}:Prose{id=#1}: masonry::passes::update: Prose received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#8}:SizedBox{id=#7}:Flex{id=#6}:Flex{id=#3}:Label{id=#2}: masonry::passes::update: Label received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#8}:SizedBox{id=#7}:Flex{id=#6}:Flex{id=#5}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#8}:SizedBox{id=#7}:Flex{id=#6}:Flex{id=#5}:VariableLabel{id=#4}: masonry::passes::update: VariableLabel received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#17}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#17}:Button{id=#10}: masonry::passes::update: Button received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#17}:Button{id=#10}:Label{id=#9}: masonry::passes::update: Label received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#17}:Button{id=#12}: masonry::passes::update: Button received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#17}:Button{id=#12}:Label{id=#11}: masonry::passes::update: Label received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#17}:Button{id=#14}: masonry::passes::update: Button received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#17}:Button{id=#14}:Label{id=#13}: masonry::passes::update: Label received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#17}:Button{id=#16}: masonry::passes::update: Button received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Flex{id=#17}:Button{id=#16}:Label{id=#15}: masonry::passes::update: Label received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}: masonry::passes::update: Portal received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}:Flex{id=#160}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}:Flex{id=#160}:SizedBox{id=#24}: masonry::passes::update: SizedBox received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}:Flex{id=#160}:SizedBox{id=#24}:Flex{id=#23}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}:Flex{id=#160}:SizedBox{id=#24}:Flex{id=#23}:Flex{id=#20}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}:Flex{id=#160}:SizedBox{id=#24}:Flex{id=#23}:Flex{id=#20}:Prose{id=#18}: masonry::passes::update: Prose received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}:Flex{id=#160}:SizedBox{id=#24}:Flex{id=#23}:Flex{id=#20}:Label{id=#19}: masonry::passes::update: Label received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}:Flex{id=#160}:SizedBox{id=#24}:Flex{id=#23}:Flex{id=#22}: masonry::passes::update: Flex received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}:Flex{id=#160}:SizedBox{id=#24}:Flex{id=#23}:Flex{id=#22}:VariableLabel{id=#21}: masonry::passes::update: VariableLabel received Update::WidgetAdded
14:37:40.365Z TRACE update_new_widgets:RootWidget{id=#165}:Flex{id=#164}:Portal{id=#163}:Flex{id=#160}:SizedBox{id=#31}: masonry::passes::update: SizedBox received Update::WidgetAdded
```

</p>
</details> 

This was originally an experiment into caching spans, but I determined
that was non-viable due to the pass names.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants