Remove SceneBuilder/Fragment #432
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In a move toward dropping the SceneFragment and SceneBuilder types, this unifies the underlying encoding of Scene and SceneFragment. Specifically, we no longer encode an initial transform and style for Scene. This causes the transform and style indices in the path tag monoid to be off by one. I chose the simple approach of subtracting 1 from each in the combine_tag_monoid() functions in both GPU and CPU code to correct this.
This removes both `SceneBuilder` and `SceneFragment` while moving all of the "builder" methods directly to `Scene`. Based on #426 which must land first.
armansito
approved these changes
Feb 13, 2024
| @@ -376,7 +376,7 @@ fn run( | |||
| height, | |||
| antialiasing_method, | |||
| }; | |||
| let mut builder = SceneBuilder::for_scene(&mut scene); | |||
| scene.reset(); | |||
There was a problem hiding this comment.
I like that this is explicit now (instead of implicitly resetting the scene by creating a new fragment.
There was a problem hiding this comment.
This was a definite pain point and we have the complaints to prove it :)
| pub struct Scene { | ||
| data: Encoding, | ||
| encoding: Encoding, |
DJMcNab
reviewed
Feb 13, 2024
examples/scenes/src/lib.rs
Outdated
| @@ -43,11 +43,11 @@ pub struct ExampleScene { | |||
| } | |||
|
|
|||
| pub trait TestScene { | |||
| fn render(&mut self, sb: &mut SceneBuilder, params: &mut SceneParams); | |||
| fn render(&mut self, sb: &mut Scene, params: &mut SceneParams); | |||
There was a problem hiding this comment.
sb here is short for SceneBuilder, which is of course no longer accurate
I'm not certain what we should change it to. Maybe scene would be fine?
There was a problem hiding this comment.
Oops, good catch. Renamed all of these (and occurrences of builder) to scene.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes both
SceneBuilderandSceneFragmentwhile moving all of the "builder" methods directly toScene. Also derivesCloneforScene.Fixes #342 and subsumes #243.
Based on #426 which must land first.