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

fix: respect devicePixelRatio during simplification & avoid needless simplification/projection cache invalidation #1812

Merged
merged 6 commits into from
Feb 6, 2024

Conversation

JaffaKetchup
Copy link
Member

@JaffaKetchup JaffaKetchup commented Jan 25, 2024

Fixes an issue where devices with higher devicePixelRatios are less simplified. Also improved the cache invalidation algorithm to clear cache more accurately, only when each component requires it.

ignatz and others added 3 commits January 25, 2024 11:44
…ed simplifications

Refactored internal projection & simplification caching logic into internal `PSCachingLayerState` mixin
@JaffaKetchup JaffaKetchup changed the title fix: respect DPI during simplificiation fix: respect devicePixelRatio during simplificiation Jan 25, 2024
Reworked `devicePixelRatio` cache invalidator
@JaffaKetchup
Copy link
Member Author

Hey @ignatz,
Unfortunately this doesn't appear to have improved the performance on high-DPI screen to equivalent to the lower-DPI. Is there any reason this could be? I can confirm that the simplification value is changing.

@ibrierley
Copy link
Collaborator

Whilst the simplifcation value may have matched, are the lines thickness now more than 1px at all ? (sorry not been following the code to dig)

@JaffaKetchup
Copy link
Member Author

Even if they were (they shouldn't be), I wouldn't expect to see it change across the same device just different screens.

It's worth noting though that, indeed, borders of more than 1px are massively slower than 1px lines (when testing the drawVertices PR) - unfortunately, using drawRawPoints only seemed to make it worse.

@ignatz
Copy link
Contributor

ignatz commented Jan 25, 2024

Whilst the simplifcation value may have matched, are the lines thickness now more than 1px at all ? (sorry not been following the code to dig)

You just blew my mind - thanks. It does seem line thickness related. Would have never guessed and probably taken hours.

For context: at first I thought that clearly the render thread takes longer because the polygons are less simplified, i.e. there's just more to draw. I tried to rectify with this change.

However, if I change my pixel ratio w/o restarting the App, i.e. the cached point are exactly the same, the render thread still buckles even though it has to draw the same amount of polygon lines. Changing the stroke width to something small (i took 0.2 because 2*0.2 << 1) makes all the difference:

devicePixelRatio of 2:

Screenshot from 2024-01-25 21-36-01

devicePixelRatio of 1:

Screenshot from 2024-01-25 21-36-32

Whilst the simplifcation value may have matched, are the lines thickness now more than 1px at all ? (sorry not been following the code to dig)

Even if they were (they shouldn't be), I wouldn't expect to see it change across the same device just different screens.

🤷 my little experiment seems to suggest that they do.

It's worth noting though that, indeed, borders of more than 1px are massively slower than 1px lines (when testing the drawVertices PR) - unfortunately, using drawRawPoints only seemed to make it worse.

That's a bit on a tangent but I also concluded that drawRawPoints is rather pointless 🤣 . We discussed that a while back in discord.

@JaffaKetchup
Copy link
Member Author

Ah, I see now. I didn't think it through fully (sorry @ibrierley): of course, the actual pixel thickness of the outlines are changing as well, due to the DPR. Is there any way we can fix this - should we be making a border stroke width of 0 mean 1 device px on any device (as Divider does)?


That's a bit on a tangent but I also concluded that drawRawPoints is rather pointless 🤣 . We discussed that a while back in discord.

Bit on a tangent but should have immediately made it clear to me that @ibrierley was right, given that I'd seen this exact same effect less than 24 hours earlier :D

@ibrierley
Copy link
Collaborator

ibrierley commented Jan 25, 2024

So this issue is pretty much due to a different render path/calculations (not sure correct term) being used for pixels over 1px by Skia (was wondering if Impeller would be different, but it may face the same issues, and just rereading the thread below I'm guessing not).

Basically I think its more calculations need to be made as it needs to take into account linecaps and joins if its over 1px, which is can skip for skinny lines.

This is worth a read, as I discuss it a bit with flutter a couple of years ago, but hoped they would fix it one way or another.
flutter/flutter#78543

I had pondered this a lot with vectors tiles, and kinda kept hitting a brick wall. I felt like maybe the best way would be some hybrid solution of polygons and very basic tesselation (eg https://www.codeproject.com/Articles/226569/Drawing-polylines-by-tessellation ) but realistically I suspect any extra calculations just get you back to the same issue of losing performance, so didn't bother taking it any further (if it was in opengl maybe it would be worth it, but then maybe that has its own other issues).

Another thought, a lot of that thread seemed specifically slow at dragging transformed objects, whether some hackery can be got around to fake that, or more simplification when dragging, or is that a bit ugly...

@JaffaKetchup
Copy link
Member Author

It does appear as though, from rather limited testing on my mid-low end Android device, Impeller significantly improves the painting of lines. I tested this through #1800 with the performant rendering pathway enabled and disabled. Whether the improvements of #1800 were dwarfed by Impeller, I'm unsure, and further testing is needed. But keeping it to the scope of this PR, it does appear as though Impeller helps a lot.

Do we want to add a hairline border feature (1 logical pixel = 1 device pixel) somehow? And/or do we want to wait for Impeller to become stable?

@JaffaKetchup JaffaKetchup marked this pull request as ready for review January 31, 2024 21:56
@JaffaKetchup
Copy link
Member Author

Anyway, as that is a seperate issue, and I think this PR still fixes an issue, I'm marking this as ready for review.

@JaffaKetchup JaffaKetchup changed the title fix: respect devicePixelRatio during simplificiation fix: respect devicePixelRatio during simplification & less eagerly invalidate simplification and projection caches on feature layers Jan 31, 2024
@JaffaKetchup JaffaKetchup changed the title fix: respect devicePixelRatio during simplification & less eagerly invalidate simplification and projection caches on feature layers fix: respect devicePixelRatio during simplification & avoid needless simplification/projection cache invalidation Jan 31, 2024
@ignatz
Copy link
Contributor

ignatz commented Feb 1, 2024

Anyway, as that is a seperate issue, and I think this PR still fixes an issue, I'm marking this as ready for review.

I certainly doesn't hurt and helps a little.

It does appear as though, from rather limited testing on my mid-low end Android device, Impeller significantly improves the painting of lines. I tested this through #1800 with the performant rendering pathway enabled and disabled.

That's nice to hear. I guess it's expected that it at least performs differently since it shares hardly any code with Skia.

Whether the improvements of #1800 were dwarfed by Impeller, I'm unsure, and further testing is needed. But keeping it to the scope of this PR, it does appear as though Impeller helps a lot.

I'd expect them to be additive, so drawVertices is still an improvement at least for the fills.

Do we want to add a hairline border feature (1 logical pixel = 1 device pixel) somehow? And/or do we want to wait for Impeller to become stable?

Personally, I wouldn't add a new feature. I'd rather:

  1. redefine that the border is given in physical pixels
  2. or document that users can divide by device pixel ratio themselves if they value performance over visual proportionality

🤷

josxha

This comment was marked as outdated.

@josxha josxha added this to the v7.0 milestone Feb 2, 2024
@JaffaKetchup
Copy link
Member Author

See flutter/flutter#131345 (comment) for some performance comparisons regarding the line thickness.

Copy link
Contributor

@josxha josxha left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@JaffaKetchup JaffaKetchup merged commit fa4393e into master Feb 6, 2024
8 checks passed
@JaffaKetchup JaffaKetchup deleted the fix-simplification-dpi branch February 6, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants