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

Snap the TextBox cursor to the pixel grid #1794

Merged
merged 1 commit into from
Jun 30, 2021

Conversation

CryZe
Copy link
Contributor

@CryZe CryZe commented May 19, 2021

This snaps the cursor of the TextBox to the grid to ensure it's always rendered as sharp as possible.

Screenshot:

Top is with grid snapping, bottom without.

https://i.imgur.com/3YixR6D.png

@CryZe CryZe force-pushed the snap-cursor-to-grid branch 2 times, most recently from 24a5883 to eb9023e Compare May 19, 2021 16:04
This snaps the cursor of the TextBox to the grid to ensure it's always
rendered as sharp as possible.
@SecondFlight SecondFlight added the S-needs-review waits for review label May 19, 2021
Copy link
Collaborator

@xarvic xarvic 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!

@xarvic xarvic added S-ready PR is ready to merge and removed S-needs-review waits for review labels May 20, 2021
@cmyr
Copy link
Member

cmyr commented May 21, 2021

I'd like to take a look at this before it gets merged please. (I can't figure out how to self-request a review from my phone 😕)

@xarvic xarvic requested a review from cmyr May 21, 2021 09:44
@xarvic xarvic added S-needs-review waits for review and removed S-ready PR is ready to merge labels May 21, 2021
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Thanks for this! I do think it's important to get this stuff right, and my main concern is totally unrelated to this patch, and is more about how need some mechanisms for being more consistent about this, across the board. There's a discussion in #1088, and there's been various efforts in the past (linebender/kurbo#93, linebender/kurbo#107, #904).

Anyway, main actual question is whether we should round to trunc?

Comment on lines +616 to +617
cursor.p0.x = cursor.p0.x.trunc() + 0.5;
cursor.p1.x = cursor.p0.x;
Copy link
Member

Choose a reason for hiding this comment

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

  • this should be correct for coregraphics and skia; i'm not sure if directwrite behaves the same way?
  • might round make more sense than trunc?
  • I'm actually also not sure if this will work at all scale factors. it should work at 1x and 2x? I'm less sure about other cases.

All that said: I'm not opposed to this at all, it's just exposing some of my general confusion about pixels and scale factor. I would really love to see a sort of 'unified' theory of this stuff; until we have that it's going to feel like our drawing code is an inconsistently applied series of heuristics. :/

@cmyr
Copy link
Member

cmyr commented Jun 30, 2021

I'm going to merge this, it's an improvement on the current code and we can always fix other problems later.

@cmyr cmyr merged commit e2bb4fb into linebender:master Jun 30, 2021
@xStrom xStrom removed the S-needs-review waits for review label Jan 19, 2023
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