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

[Draft] Update dynamic_font.cpp To fix Flickering #39655

Closed
wants to merge 1 commit into from
Closed

[Draft] Update dynamic_font.cpp To fix Flickering #39655

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 18, 2020

Hello, this is a fix for the bug i reported here #39628

@Calinou Calinou added bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:gui labels Jun 19, 2020
@Calinou Calinou added this to the 4.0 milestone Jun 19, 2020
@akien-mga
Copy link
Member

Doesn't this break font oversampling, instead of fixing it? There's more code that refers to it using floats and does float comparisons.

The commit log should be amended to be more explicit, see https://github.com/godotengine/godot/blob/master/CONTRIBUTING.md#format-your-commit-messages-with-readability-in-mind

@ghost
Copy link
Author

ghost commented Jun 19, 2020

Sorry , its My first time ever to use the pull requests system.

From tracking the bug i reported , I find out that the font_oversampling is float value of 1.0
I failed to find if this value changes anywhere.

I have done my tests , I find only 1 downsides of this,
When scalling down the size , the oversamlping value doesn't scale down.
Neather when scalling up.

That's all i have for know.

@akien-mga
Copy link
Member

See this search for a list of locations where font_oversampling is used:

main/main.cpp
1947:                   bool font_oversampling = GLOBAL_DEF("rendering/quality/dynamic_fonts/use_oversampling", true);
1948:                   sml->get_root()->set_use_font_oversampling(font_oversampling);

scene/main/window.cpp
540:    float font_oversampling = 1.0;
619:                            font_oversampling = screen_size.x / viewport_size.x;
645:                    font_oversampling = 1.0;
647:            if (DynamicFontAtSize::font_oversampling != font_oversampling) {
648:                    DynamicFontAtSize::font_oversampling = font_oversampling;

scene/resources/dynamic_font.cpp
220:float DynamicFontAtSize::font_oversampling = 1.0;
633:    if (oversampling == font_oversampling || !valid) {
640:    oversampling = font_oversampling;
651:    oversampling = font_oversampling;

scene/resources/dynamic_font.h
182:    static float font_oversampling;

I'm not an expert on it myself so maybe this change will be OK, but it sounds to me like it will create issue whenever one would actually need a ratio different from 1. I guess this change will mean that values inferior to 1 (e.g. 0.7) will be rounded back to 1, and there will be a jump from 1 to 2 when the scale ratio passes 1.5.

@akien-mga akien-mga requested a review from Calinou June 19, 2020 11:27
@ghost
Copy link
Author

ghost commented Jun 19, 2020

As far as i can see , the float values are the reason for the problem.
But the default font doesn't have this problem.
so the problem is deeper then just oversampling value.

@volzhs
Copy link
Contributor

volzhs commented Jun 21, 2020

AFAIK, the default font is bitmap font not dynamic font.
So it is not related to oversampling.

@ghost ghost changed the title Update dynamic_font.cpp To fix Flickering [Draft] Update dynamic_font.cpp To fix Flickering Sep 18, 2020
@akien-mga
Copy link
Member

OP has deleted their account and this change still seems bogus to me, so closing. The related issue is still open for investigating and fixing (possibly with the same fix if it turns out to be correct after further investigation).

@akien-mga akien-mga closed this Feb 25, 2021
@akien-mga akien-mga added archived and removed cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Feb 25, 2021
@Calinou
Copy link
Member

Calinou commented Feb 25, 2021

The font oversampling factor should remain a floating-point value as the scaling factor won't always be an integer (even if the final DynamicFont size is always an integer). Therefore, I don't think this pull request was a good solution to the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants