-
-
Notifications
You must be signed in to change notification settings - Fork 867
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
Conversation
…ed simplifications Refactored internal projection & simplification caching logic into internal `PSCachingLayerState` mixin
… and simplification
devicePixelRatio
during simplificiation
Reworked `devicePixelRatio` cache invalidator
Hey @ignatz, |
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. It's worth noting though that, indeed, borders of more than 1px are massively slower than 1px lines (when testing the |
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: devicePixelRatio of 1:
🤷 my little experiment seems to suggest that they do.
That's a bit on a tangent but I also concluded that drawRawPoints is rather pointless 🤣 . We discussed that a while back in discord. |
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
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 |
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. 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... |
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? |
Anyway, as that is a seperate issue, and I think this PR still fixes an issue, I'm marking this as ready for review. |
devicePixelRatio
during simplificiationdevicePixelRatio
during simplification & less eagerly invalidate simplification and projection caches on feature layers
devicePixelRatio
during simplification & less eagerly invalidate simplification and projection caches on feature layersdevicePixelRatio
during simplification & avoid needless simplification/projection cache invalidation
I certainly doesn't hurt and helps a little.
That's nice to hear. I guess it's expected that it at least performs differently since it shares hardly any code with Skia.
I'd expect them to be additive, so drawVertices is still an improvement at least for the fills.
Personally, I wouldn't add a new feature. I'd rather:
🤷 |
See flutter/flutter#131345 (comment) for some performance comparisons regarding the line thickness. |
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.
lgtm 👍
Fixes an issue where devices with higher
devicePixelRatio
s are less simplified. Also improved the cache invalidation algorithm to clear cache more accurately, only when each component requires it.