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

Improve overlay rendering for DPI scaling #2073

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Zamoca42
Copy link

@Zamoca42 Zamoca42 commented Oct 24, 2024

Fixes #1688

Changes

Apply the following changes to the overlay using device_pixel_ratio:

  1. Pixel alignment
  2. Grid spacing adjustment
  3. Dot size adjustment

Preview

dpr 1

dpr1

dpr 2

dpr2

dpr 3

dpr3

@Keavon
Copy link
Member

Keavon commented Oct 24, 2024

Thanks! Just an observation from the screenshots, it looks like the stroke diameter gets smaller and smaller but should be rounded to the nearest pixel scale.

@Zamoca42
Copy link
Author

Yeah, I changed the outline to a linear scale based on DPR, but it caused dramatic of a change. So, I applied pixel scaling by rounding to the first decimal place on the root scale instead.

  1. DPR 1
    dpr1

  2. Linear scale at DPR 3
    dpr3

  3. Square root scale at DPR 3
    dpr3-root

@Zamoca42 Zamoca42 marked this pull request as ready for review November 1, 2024 15:04
@Zamoca42
Copy link
Author

Zamoca42 commented Nov 1, 2024

I have a question while implementing this.

Is it necessary to change the size of the Overlay Canvas based on DPR (Device Pixel Ratio)?

I thought that simply increasing the physical pixel count of the Overlay Canvas would result in a clearer overlay, but this creates size and position discrepancies with the regular canvas.

@Keavon
Copy link
Member

Keavon commented Nov 1, 2024

I don't recall off-hand if that is necessary but it could be worth attempting. I'll delegate that to you as a research/experiment and I can code review it afterwards to sanity check. If you get really stuck on that, I can try to find a bit of time to help research it.

@Zamoca42
Copy link
Author

Zamoca42 commented Nov 3, 2024

Thanks! What I tried was simply converting logical pixels to physical pixels on the canvas and applying the scale in the context. However, it seems that the scale in the context isn’t applied during resize or move. Is there a way to apply the scale to the overlay’s events as well?

Edit: When running in the development environment, I noticed a "Snap candidate overflow" occurring at /src/messages/tool/common_functionality/snapping.rs:349, which appears to prevent context.scale from being applied correctly. This issue occurs even without physical pixel adjustments on the overlay canvas, so the logic in snapping.rs needs to be revised.

I’m attaching a video of the overlay canvas with physical pixels applied.

resize.mov

@Zamoca42
Copy link
Author

Zamoca42 commented Nov 9, 2024

Applying physical pixels did not result in noticeable rendering differences, and I also identified an issue with inaccuracies occurring during the context.scale configuration of the overlay.

Given this, I concluded that applying physical pixels does not provide substantial improvements to the project and could add unnecessary complexity from a maintenance perspective.

Therefore, I have determined that applying physical pixels is not necessary at this stage.

Thank you for giving me the time to review this.

@Keavon
Copy link
Member

Keavon commented Nov 21, 2024

I will try to dive into the details of this in a couple weeks when I have time. Unfortunately the issue hasn't been the most pressing for me, since I run a monitor with 100% display scaling, so it's been difficult to prioritize so far. Sorry about that! I appreciate your attention towards this task even though I've somewhat neglected it. It would also be helpful if you could summarize the current state of things, and your research into the prior questions, so I can have the necessary context once I return to this. Thank you!

@Zamoca42
Copy link
Author

Zamoca42 commented Nov 26, 2024

OK, Please take all the time you need. Let me summarize our progress so far:

  1. Attempted to apply display scaling to overlays using window.device_pixel_ratio within the overlay context.
  2. Tried applying physical pixels to the overlay canvas to test for any improvement in sharpness. However, despite scaling proportionally using context.scale to account for the physical pixels, it did not work correctly at certain points.
    See above resize.mov.
  3. Discovered an overflow issue in the find_candidate function of snapping.rs for Snap candidates and modified the logic to resolve it.
  4. Applied scaling to all overlay functions associated with tools (e.g., draw_overlays function in editor/src/messages/tool/common_functionality/snapping.rs), but scaling still did not work as intended.

Additionally, the code for the overlay canvas with physical pixels applied has not yet been removed.

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