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.

Sign up for GitHub

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

Jump to bottom

fix text legibility with subpixel vertex position #151

Closed
wants to merge 1 commit into from
Closed

fix text legibility with subpixel vertex position #151

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion crates/yakui-core/src/paint/paint_dom.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@ use crate::dom::Dom;
use crate::geometry::Rect;
use crate::id::{ManagedTextureId, WidgetId};
use crate::layout::LayoutDom;
use crate::paint::{PaintCall, Pipeline};
use crate::widget::PaintContext;

use super::layers::PaintLayers;
use super::primitives::{PaintCall, PaintMesh, Vertex};
use super::primitives::{PaintMesh, Vertex};
use super::texture::{Texture, TextureChange};

/// Contains all information about how to paint the current set of widgets.
Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might need to figure out how to move the scale factor multiplication out of the pipeline if we wanted to resolve this

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For rounded rect:
first: rounded
second: unrounded

we think this issue is even more pronounced with a non-integer scale factor

image
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

rounded, with a scale factor of 1.25

pos = (pos * self.surface_size).round() / self.surface_size;
Copy link
Member

Choose a reason for hiding this comment

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

Since we divide by surface_size above, can we move this up statement a line and just write:

pos = pos.round();

Copy link
Member

Choose a reason for hiding this comment

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

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;

}

vertex.position = pos;
vertex
});
Expand Down
Loading