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 blurry overlay rendering when the pixel display ratio isn't 100% #2204

Merged
merged 2 commits into from
Jan 25, 2025

Conversation

andystopia
Copy link
Contributor

@andystopia andystopia commented Jan 18, 2025

Closes #1688 . Supersedes #2073.

I have used Graphite occasionally over the past couple of months, and I have really enjoyed my experience, and I wanted to give back a little :) This implementation makes text more sharp on my MacBook. I believe it to be related to issue #1688 and PR #2073. I believe it correctly implements what's mentioned in #1688 as far as scaling and rounding are concerned, but for #2073, I couldn't understand the bug with snapping, so I think that would make sense to be continued in that PR.

Visual Comparison

graphite.rs fork
image image

Limitations

  • Lack of abstraction and RAII leads to repeated adjustments to transformation matrix
    within the OverlayContext.
  • I know I haven't fully explored the features of the editor, and that if I missed configuring the context somewhere,
    rendering could be offset by 1/device pixel ratio to the top left.
  • Snapping bug will need more research in Improve overlay rendering for DPI scaling #2073.
  • Grid Overlays HiDPI seems to be implemented in Improve overlay rendering for DPI scaling #2073, but I don't use that feature, so I'm not sure if this will break them.

@Keavon
Copy link
Member

Keavon commented Jan 18, 2025

Thank you for this! I will try to review it soon. Can you please clarify a bit more about the relationship between what you could understand about #2073 and its comparison to yours? This is a clean slate, not a derivative, implementation, correct? I unfortunately don't have the best memory of its status but I recall certain things not working properly when I tested which, combined with my lack of time and concerns about how much code was needed to be changed, is why I unfortunately didn't actively work on it so far. I will test if this version just works as-is, and I see that it touches fewer lines of code as well which is good for simplicity.

@Keavon
Copy link
Member

Keavon commented Jan 18, 2025

!build

Copy link

📦 Build Complete for 51c4c37
https://e0f975a0.graphite.pages.dev

@andystopia
Copy link
Contributor Author

andystopia commented Jan 18, 2025

Can you please clarify a bit more about the relationship between what you could understand about #2073 and its comparison to yours?

My implementation is intended to be minimal and extensible. I think that #2073 is implementing more features, while I hope that my implementation will work out of the box now, in preparation for a more detailed implementation later, if need be. I'm sure the #2073 has a lot of value, but, unfortunately, I couldn't figure out how to make the borders and text size line up using that implementation.

This is a clean slate, not a derivative, implementation, correct?

Correct :D

I unfortunately don't have the best memory of its status but I recall certain things not working properly when I tested which, combined with my lack of time and concerns about how much code was needed to be changed, is why I unfortunately didn't actively work on it so far.

Makes sense. I hope there isn't too much to test in mine. It's mostly a lot of setting matrix transforms. The pages deploy looks how I'd expect :)

From #2073:

Is this something that would make sense to break out into its own PR for a swifter merge process?

This is why I wrote this clean slate. I wanted something small and that can be merged quickly.

I will test if this version just works as-is, and I see that it touches fewer lines of code as well which is good for simplicity.

If it doesn't, I'm sorry, I tried hard to make it work as-is :D

@Keavon
Copy link
Member

Keavon commented Jan 18, 2025

Thanks for the info. Does your implementation work on the basis of utilizing the new (denser) pixel grid in the pixel-perfect drawing of overlays so antialiasing is avoided? We currently take pains to ensure all drawing is aligned perfectly with the grid so nothing looks fuzzy. But we take assumptions about the 100% scaling ratio of the display and OS + browser scale factor.

@andystopia
Copy link
Contributor Author

andystopia commented Jan 18, 2025

My implementation creates an even-width canvas that has enough pixels to adjust for devicePixelRatio. My implementation is essentially based on the one that Mozilla provides in the devicePixelRatio Docs. Essentially, create a canvas that is devicePixelRatio * cssWidth pixels wide to adjust for pixel density, then apply set a transformation matrix on the canvas before drawing.

On my device, I have a devicePixelRatio (dpr) of 2.0, and if I want to draw a line with one point at (19, 19), then after the transformation, I believe it is actually drawing it at (36, 36). My intuition tells me that most things working off CSS pixels will probably stay pixel aligned, because $\forall v. v \in \mathbb{Z}, 2v \in \mathbb{Z}$, but I could be thinking about this incorrectly.

If I change my browser scale then my dpr also changes, often to a non-integer value. I think a dpr of 1.5 would cause (19, 19) to actually start drawing at (28.5, 28.5). I could probably implement the matrix transformations myself, so that I can grab that (28.5, 28.5) and round to (29, 29) or (28, 28), but that implementation is much more in depth and doesn't follow the beaten path provided by MDN.

My reasoning for this change is not that it's a perfect solution right now, but it is a simple one that works relatively well. If you're on an dpr=1, dpr=2, or dpr=3 display then the lack of AA will be preserved, and the text will be sharper, and if you're on a non-integer dpr display, then maybe you'll encounter some misalignment caused AA, but the text will appear more crisp.

What way would you like to take this? I'm open to either approach :D

@Keavon Keavon changed the title Support HiDPI Overlay Rendering Fix blurry overlay rendering when the pixel display ratio isn't 100% Jan 25, 2025
Copy link
Member

@Keavon Keavon left a comment

Choose a reason for hiding this comment

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

Thank you for this. It took me a little while but your implementation was simple enough that I was able to spend the time to review and land it, with a few changes (I made it watch for changes in scale and update live).

@Keavon Keavon enabled auto-merge (squash) January 25, 2025 09:25
@Keavon Keavon merged commit 33ac141 into GraphiteEditor:master Jan 25, 2025
4 checks passed
@Keavon
Copy link
Member

Keavon commented Jan 25, 2025

@andystopia please leave a comment in #1688 saying you completed it in this PR. This is needed so your comment lets me assign that issue to you.

@andystopia
Copy link
Contributor Author

Thank you Keavon, I'm glad that you were able to extend it to update live! That will definitely be much appreciated on my side :D I have left the comment as requested :)

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.

Fix DPI display scaling to make overlays render at the native resolution
2 participants