Correctly handle disjoint bounding-box intersections in binning #335
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
Added a test scene that demonstrates the clipping bugs reported in vello#286 and vello#333.
Closed
raphlinus
approved these changes
Jun 29, 2023
shader/binning.wgsl
Outdated
| @@ -89,12 +89,18 @@ fn main( | |||
| let bbox_raw = bbox_intersect(clip_bbox, pb); | |||
| // TODO(naga): clunky expression a workaround for broken lhs swizzle | |||
| let bbox = vec4(bbox_raw.xy, max(bbox_raw.xy, bbox_raw.zw)); | |||
There was a problem hiding this comment.
You probably no longer need the max here, as everything downstream should be able to interpret x1 < x0 or y1 < y0 as an empty box.
|
Yes! This has been in my queue for a long time and I’ve been dreading hunting it down. I’m excited to test this on some Lottie scenes with known clipping artifacts. |
When the bounding boxes of a path and its clip are disjoint (i.e. they do not intersect) the result of their intersection is a negative rectangle. When calculating the intersection of the bboxes, the binning stage ensures that the bbox is non-negative. It then normalizes the coordinates to bin dimensions and rounds the top-left corner down to the neareast integer and the bottom-right corner up. However this rounding causes zero-area bounding boxes to have a non-zero area and sends the bottom-right corner to the placed in the next bin. This causes fully clipped out draw objects to be included in binning, with an incorrect clip bounding box that causes them to be erroneously drawn with partial clipping. `binning` now takes care around this logic to make sure that a zero-area intersected-bbox gets skipped and clipped out. `tile_alloc`, also takes care in its logic. Fixes linebender#286 and linebender#333
@dfrg Yes, please do! Let me know if this fixes the artifacts you've been seeing with lottie scenes. |
raphlinus
added a commit
that referenced
this pull request
Oct 6, 2023
Basically ports #335, which aggressively culls empty bounding boxes.
raphlinus
added a commit
that referenced
this pull request
Oct 7, 2023
Basically ports #335, which aggressively culls empty bounding boxes.
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.
When the bounding boxes of a path and its clip are disjoint (i.e. they do not intersect) the result of their intersection is a negative rectangle.
When calculating the intersection of the bboxes, the binning stage ensures that the bbox is non-negative. It then normalizes the coordinates to bin dimensions and rounds the top-left corner down to the neareast integer and the bottom-right corner up.
However this rounding causes zero-area bounding boxes to have a non-zero area and sends the bottom-right corner to be placed in the next bin. This causes fully clipped out draw objects to be included in binning, with an incorrect clip bounding box that causes them to be erroneously drawn with partial clipping.
binningnow takes care around this logic to make sure that a zero-area intersected-bbox gets skipped and clipped out.tile_alloc, also takes care in its own similar logic over intersected-bbox contents.Fixes #286 and #333