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

Invalidate font atlas on any change to pixels_per_point, not matter how small #3698

Merged
merged 1 commit into from Jan 8, 2024

Conversation

StarStarJ
Copy link
Contributor

Apparently the font implementation uses a distance check to decide if the font(or whatever) need recalculations, after dpi changed:

let pixels_per_point_changed =
(fonts_and_cache.fonts.pixels_per_point - pixels_per_point).abs() > 1e-3;

This leads to warnings when the pixel_per_point diff is very low and spams the log. (<- this happens for me if i resize my window on kwin, e.g. maximize it)

(I don't want to debate if the float difference generally makes sense, so if you want to rework that instead just close this pr)

@emilk
Copy link
Owner

emilk commented Jan 6, 2024

This seems to be fixing a symptom rather than the problem. Have you investigated why the float difference happen at all? I don't hit this assert.

@StarStarJ
Copy link
Contributor Author

StarStarJ commented Jan 7, 2024

I don't know if i want to dig into the details of winit & x11, but this value is reported by winit, and it's definitely changing by this small amount.

But besides that, if for whatever reason the DPI changes by a very small amount, what does egui do to handle it?

the mentioned code snipset in my comment shows that it ignores small differences. the warning that I i fixed in this pr does not. So these two code segments behave different on the same logic, or not?

@emilk
Copy link
Owner

emilk commented Jan 7, 2024

But where does the change come from? Are you storing a Galley between frames (you shouldn't)?

If you create the galley in one frame, and then tesslleate, then I can't see how the pixels_per_point could change.

Have you tried latest master?

@StarStarJ
Copy link
Contributor Author

But where does the change come from? Are you storing a Galley between frames (you shouldn't)?

Well the Galley comes from from the font:

impl FontsAndCache {
fn layout_job(&mut self, job: LayoutJob) -> Arc<Galley> {
self.galley_cache.layout(&mut self.fonts, job)
}
}

I honestly dunno how exactly to debug if these Galley are stored between frames, but a GalleyCache sounds like the implementation does that?

I have to say I don't have the insight into egui, the docs for the Galley also only say:
"Needs to be recreated if the underlying font atlas texture changes, which happens under the following conditions: [...]"

(https://docs.rs/egui/latest/egui/struct.Galley.html)

I am not manually recreating my fonts if pixels_per_point change, but the doc sounds like it's doing that internally anyway in crate::text::Fonts::begin_frame

Then bcs of:

let pixels_per_point_changed =
(fonts_and_cache.fonts.pixels_per_point - pixels_per_point).abs() > 1e-3;

The pixels_per_point change is ignored (because of the floating point difference)
=> galley.pixels_per_point

but the line in this pr compares it again self.pixels_per_point which IS changed
(galley.pixels_per_point: 1.16666663 vs. self.pixels_per_point: 1.16666675 (so very very small => galley is not updated in

let pixels_per_point_changed =
(fonts_and_cache.fonts.pixels_per_point - pixels_per_point).abs() > 1e-3;

but the warning is still displayed
)

I'm indeed not using latest master, if you say Galley is never cached between frames, then it's indeed a bug in my application. I am using custom fonts, but i am not sure which font is triggering this issue.

@emilk
Copy link
Owner

emilk commented Jan 7, 2024

Ah sorry, I finally follow… pixels_per_point_changed uses a 1e-3 epsilon (for whatever reason), so of course we need a similar (or great) epsilon when we are warning. Let me think through this a bit and I'll either merge your PR or come up with something better before next release.

@emilk emilk added this to the Next Major Release milestone Jan 7, 2024
@emilk
Copy link
Owner

emilk commented Jan 7, 2024

Does

-        let pixels_per_point_changed =
-            (fonts_and_cache.fonts.pixels_per_point - pixels_per_point).abs() > 1e-3;
+        let pixels_per_point_changed = fonts_and_cache.fonts.pixels_per_point != pixels_per_point;

solve your problem?

I really don't see the point of the 1e-3 epsilon there

@StarStarJ
Copy link
Contributor Author

Yes that works just as good 👍

@emilk emilk changed the title Make epaint pixel_per_point warning only trigger on same float diff as fonts is doing Invalidate font atlas on any change to pixels_per_point, not matter how small Jan 8, 2024
@emilk emilk added the egui label Jan 8, 2024
@emilk emilk merged commit 7b105cf into emilk:master Jan 8, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants