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

Ensure that damage is non-overlapping #1198

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

kchibisov
Copy link
Member

@kchibisov kchibisov commented Nov 4, 2023

The previous logic was blindly merging and doing only a forward pass, however this won't work when after merging 2 rectangles already processed one could end up in overlap.

This is an example of old renderer doing so:

[ 697101.942] [email protected]_buffer(66, 350, 36, 48)
[ 697101.956] [email protected]_buffer(126, 446, 828, 168)
[ 697101.965] [email protected]_buffer(90, 638, 744, 528)
[ 697101.974] [email protected]_buffer(162, 1262, 420, 72)
[ 697101.984] [email protected]_buffer(114, 1358, 312, 48)
[ 697101.994] [email protected]()

damage to be rendered: [
    Rectangle<smithay::utils::geometry::Physical> {
        x: 158,
        y: 478,
        width: 828,
        height: 168,
    },
    Rectangle<smithay::utils::geometry::Physical> {
        x: 194,
        y: 1294,
        width: 420,
        height: 72,
    },
    Rectangle<smithay::utils::geometry::Physical> {
        x: 146,
        y: 1390,
        width: 312,
        height: 48,
    },
    Rectangle<smithay::utils::geometry::Physical> {
        x: 32,
        y: 382,
        width: 834,
        height: 1152,
    },
]

The new algorithm trying splits damage bounding box into non-overlapping regions containing damage, and then running tile-based damage shaping, where for each tile we try to find total intersection + union of damage rectangles.

--

I still should add tests for common patterns, but the main idea of implementation should be good.

@codecov-commenter
Copy link

codecov-commenter commented Nov 4, 2023

Codecov Report

Attention: 142 lines in your changes are missing coverage. Please review.

Comparison is base (2dedd2e) 21.98% compared to head (09b1e60) 21.85%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1198      +/-   ##
==========================================
- Coverage   21.98%   21.85%   -0.14%     
==========================================
  Files         154      155       +1     
  Lines       24093    24240     +147     
==========================================
- Hits         5298     5297       -1     
- Misses      18795    18943     +148     
Flag Coverage Δ
wlcs-buffer 18.86% <11.80%> (-0.12%) ⬇️
wlcs-core 18.52% <11.80%> (-0.15%) ⬇️
wlcs-output 7.67% <11.80%> (-0.08%) ⬇️
wlcs-pointer-input 20.62% <11.80%> (-0.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/backend/renderer/damage/mod.rs 64.28% <60.00%> (ø)
src/backend/renderer/damage/shaper.rs 8.60% <8.60%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kchibisov
Copy link
Member Author

This should be ready for review.

Copy link
Contributor

@YaLTeR YaLTeR left a comment

Choose a reason for hiding this comment

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

If I was writing this I'd absolutely add some proptest tests. Have it generate a random Vec of Rectangles of input, run the damage shaper, then verify that:

  1. All input rectangles are covered with damage.
  2. There's no overlap in the output.

src/backend/renderer/damage/shaper.rs Outdated Show resolved Hide resolved
src/backend/renderer/damage/shaper.rs Outdated Show resolved Hide resolved
src/backend/renderer/damage/shaper.rs Show resolved Hide resolved
src/backend/renderer/damage/shaper.rs Outdated Show resolved Hide resolved
src/backend/renderer/damage/shaper.rs Outdated Show resolved Hide resolved
@kchibisov
Copy link
Member Author

If I was writing this I'd absolutely add some proptest tests. Have it generate a random Vec of Rectangles of input, run the damage shaper, then verify that:

Such algorithms could generally be proved by induction, you have a routine to shape tiles, and it's being checked, and you have a top-level driver that just uses sort and find gap inside of it, so all you need to test is sort of tested already.

@kchibisov
Copy link
Member Author

All input rectangles are covered with damage.

this thing is harder than you think, because it's basically requires the same algorithm we're using, but for opaque regions.

@kchibisov kchibisov requested a review from YaLTeR November 4, 2023 16:56
The previous logic was blindly merging and doing only a forward
pass, however this won't work when after merging 2 rectangles
already processed one could end up in overlap.

This is an example of old renderer doing so:

```
[ 697101.942] [email protected]_buffer(66, 350, 36, 48)
[ 697101.956] [email protected]_buffer(126, 446, 828, 168)
[ 697101.965] [email protected]_buffer(90, 638, 744, 528)
[ 697101.974] [email protected]_buffer(162, 1262, 420, 72)
[ 697101.984] [email protected]_buffer(114, 1358, 312, 48)
[ 697101.994] [email protected]()

damage to be rendered: [
    Rectangle<smithay::utils::geometry::Physical> {
        x: 158,
        y: 478,
        width: 828,
        height: 168,
    },
    Rectangle<smithay::utils::geometry::Physical> {
        x: 194,
        y: 1294,
        width: 420,
        height: 72,
    },
    Rectangle<smithay::utils::geometry::Physical> {
        x: 146,
        y: 1390,
        width: 312,
        height: 48,
    },
    Rectangle<smithay::utils::geometry::Physical> {
        x: 32,
        y: 382,
        width: 834,
        height: 1152,
    },
]
```

The new algorithm trying splits damage bounding box into non-overlapping
regions containing damage, and then running tile-based damage shaping,
where for each tile we try to find total intersection + union of
damage rectangles.
Copy link
Collaborator

@cmeissl cmeissl left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks!

@Drakulix Drakulix merged commit e27952c into Smithay:master Nov 8, 2023
13 checks passed
@kchibisov kchibisov deleted the fix-damage-shaping branch November 8, 2023 11:05
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.

5 participants