fix text legibility with subpixel vertex position #151
Conversation
| @@ -203,6 +204,10 @@ impl PaintDom { | |||
| let mut pos = vertex.position * self.scale_factor; | |||
| pos += self.unscaled_viewport.pos(); | |||
| pos /= self.surface_size; | |||
| if mesh.pipeline == Pipeline::Text { | |||
There was a problem hiding this comment.
Why does this only apply to the text pipeline? Wouldn't other textures also be affected here?
If only text is affected, can we round the vertex positions in text_renderer instead of special casing the pipeline here? I think this is too late in the rendering pipeline to have code that specializes based on the pipeline.
There was a problem hiding this comment.
The reason it's in this stage of the pipeline is that during it we also multiply it by the scale factor, and if we remember correctly, RoundRect depends on this subpixel position to behave correctly
There was a problem hiding this comment.
We might need to figure out how to move the scale factor multiplication out of the pipeline if we wanted to resolve this
There was a problem hiding this comment.
Ah, gotcha. We need to round to the nearest physical pixel, but components output vertices in logical pixels, where it's too early in the pipeline to round.
I'd be surprised if any components relied on sub-physical-pixel vertex positions! If we can find a widget in the tree that gets worse without subpixel positions, I'm sold.
There was a problem hiding this comment.
For rounded rect:
first: rounded
second: unrounded
we think this issue is even more pronounced with a non-integer scale factor
There was a problem hiding this comment.
rounded, with a scale factor of 1.25
| @@ -203,6 +204,10 @@ impl PaintDom { | |||
| let mut pos = vertex.position * self.scale_factor; | |||
| pos += self.unscaled_viewport.pos(); | |||
| pos /= self.surface_size; | |||
| if mesh.pipeline == Pipeline::Text { | |||
| pos = (pos * self.surface_size).round() / self.surface_size; | |||
There was a problem hiding this comment.
Since we divide by surface_size above, can we move this up statement a line and just write:
pos = pos.round();There was a problem hiding this comment.
To be more specific, this code:
pos /= self.surface_size;
if mesh.pipeline == Pipeline::Text {
pos = (pos * self.surface_size).round() / self.surface_size;
}would behave identically to this code:
if mesh.pipeline == Pipeline::Text {
pos = pos.round();
}
pos /= self.surface_size;
|
When you file PRs in the future, feel free to use GitHub's "closing keywords" to make it so your PR automatically closes the issue that it fixes. For example, you can write:
and that will link your PR up with that issue and close it once I merge it. |
Co-authored-by: Madeline Sparkles <[email protected]>
|
I did kind of a roundabout squash merge here to get around the issue described here because I don't have write access to this PR branch: https://github.com/orgs/community/discussions/5634 You've been credited on that commit the same way that GitHub's automerge would've credited you, I think! I'm going to close this PR since the code made it in. |
Co-authored-by: Madeline Sparkles <[email protected]>
No description provided.