-
Notifications
You must be signed in to change notification settings - Fork 200
Fix: Properly handle scene resets to ensure correct alpha rendering #859
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
Fix: Properly handle scene resets to ensure correct alpha rendering #859
Conversation
|
cc @taj-p |
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 is reasonable.
I'd like to also make sure that @tomcur is happy with it Edit: Discussion with Tom has indicated that he will review once it has merged, if it gets in tonight
| let svg_filename = std::env::args().nth(1).expect("svg filename is first arg"); | ||
| let svg: String = | ||
| std::fs::read_to_string(svg_filename).expect("error reading file"); | ||
| let parsed_svg = PicoSvg::load(&svg, 1.0).expect("error parsing SVG"); |
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 why these three lines are inside RedrawRequested
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.
Agreed, I’ve moved it out to resumed.
Fixed in 79cb7a6
| let height = surface.config.height; | ||
| self.scene.reset(); | ||
|
|
||
| let render_scale = 5.0; |
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.
Ideally, now would be the time to change some parameter based on user input (e.g. scroll-wheel or a keyboard button to change the render_scale)
It doesn't need to have click/drag or anything else, but just something to validate that the change works correctly.
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’ve added a basic zooming behavior — it looks like everything is working as expected. I’ll refine it later by adding support for proper transformations.
Added in c9235f5
| Some(resources) => required_strips_size > resources.strips_buffer.size(), | ||
| None => true, | ||
| }; | ||
| let (needs_new_strips_buffer, needs_new_alpha_texture) = match &self.resources { |
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.
Some of this logic is quite hard to follow; there's a lot more unwrap/expect than I'd normally expect (ha). That is, I feel like the three stages of "check if anything needs to be recreated, then recreate them, then access them", could be combined better.
That being said, I don't think that refactoring that now is helpful, and so I'm happy for this to land.
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 have a similar feeling but haven’t yet come up with a better approach to managing resource preparation from None to Some and subsequently validating whether recreation is necessary. One idea is to precreate all buffers/textures in new, eliminating the need to check for None and only requiring checks for resizing buffers/textures. I think I just need to spend more time refining this and will revisit it a bit later.
Overview
This PR addresses the visual artifacts (dotted/dashed lines) that appear when rendering the same scene multiple times using the
vello_hybridrenderer.Issue Description
Previously, when resetting the scene and re-rendering repeatedly, strange visual artifacts would appear at the boundaries between shapes - specifically dotted or dashed lines along what should be properly alpha sampled boundaries. These artifacts only appeared when:
RedrawRequestedevent handlerpreparemethod was called after each re-renderRoot Cause
The root cause was a misalignment between the alpha values stored in the scene and the GPU alpha texture:
Scene::resetwas called, it correctly cleared the alphas array in the scene (reset added as a separate commit)render_svgwas called, it populated the scene with new alpha valuesRenderer::preparemethod only created a new alpha texture when the strips buffer size increasedcolvalues, but these values pointed to positions that might exceed the texture dimensionsThe critical issue was that we were only checking if the strips buffer needed to be recreated, but not independently verifying if the alpha texture needed to be recreated when only the alpha values changed.
Solution
Fixed the issue by modifying
preparemethod to:This ensures that the alpha texture always matches the current frame's alpha values, maintaining the correct relationship between strip
colindices and texture coordinates, eliminating the visual artifacts.Notes
Scene::resetmethod has been updated to properly (fully) reset the scene state by clearing all data