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 font atlas limits #6666

Closed
wants to merge 6 commits into from
Closed

Fix font atlas limits #6666

wants to merge 6 commits into from

Conversation

rparrett
Copy link
Contributor

@rparrett rparrett commented Nov 17, 2022 •
edited
Loading

Objective

Fixes #6642

Solution

Previously, entries were added to the "last used size" queue every time a single glyph was added.

Instead, now we re-use some logic that finds the font size in the queue and moves it to the front.

Caveats

Testing this with the minimal repro in #6642 revealed a new and exciting bug:

When closing a window that has a scale factor, there's a single frame where there is no longer a window / scale factor and the default is used so new font atlases are created at a different size.

At least I think that's what's going on. I don't fully understand it yet.

TODO

Made this a draft because we probably only want to do some of this work if allow_dynamic_font_sizes is true.

It also seems a bit inefficient to update the queue in add_glyph_to_atlas.

Understand the closing-window thing.

Test more thoroughly.

@rparrett rparrett marked this pull request as draft November 17, 2022 17:31
@alice-i-cecile alice-i-cecile added this to the 0.9.1 milestone Nov 17, 2022
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets labels Nov 17, 2022
@rparrett
Copy link
Contributor Author

Made this a draft because we probably only want to do some of this work if allow_dynamic_font_sizes is true.

This is more complicated than it sounds, because allow_dynamic_font_sizes can be changed at runtime. So I think it should probably be left as-is for now.

It also seems a bit inefficient to update the queue in add_glyph_to_atlas.

It looks like fixing this would require reworking process_glyphs, so I think it should probably be left as-is for now.

@rparrett
Copy link
Contributor Author

rparrett commented Nov 17, 2022 •
edited
Loading

I missed/dismissed a key point in #6642 where

  • You have exceeded max_font_atlases
  • The font atlas being removed to make room is still referenced by some text that is being displayed
  • Panic

Adding a glyph to one section of text might remove an atlas that had a handle stored in a previously computed TextLayoutInfo / PositionedGlyph.

This feels like a bit too much (for me at least, just this moment) and I think we should consider reverting #5708 and perhaps replacing it with a configurable warning threshold or something.

@rparrett rparrett closed this Nov 17, 2022
@rparrett rparrett mentioned this pull request Nov 18, 2022
Closed
bors bot pushed a commit that referenced this pull request Nov 25, 2022
@rparrett
Warn instead of erroring when max_font_atlases is exceeded (#6673)
10da4b1 
# Objective

Fixes #6642

In a way that doesn't create any breaking changes, as a possible way to fix the above in a patch release.

## Solution

Don't actually remove font atlases when `max_font_atlases` is exceeded. Add a warning instead.

Keep `TextError::ExceedMaxTextAtlases` and `TextSettings` as-is so we don't break anything.

This is a bit of a cop-out, but the problems revealed by #6642 seem very challenging to fix properly.

Maybe follow up later with something more like https://github.com/rparrett/bevy/commits/remove-max-font-atlases later, if this is the direction we want to go.

## Note

See previous attempt at a "simple fix" that only solved some of the issues: #6666
bors bot pushed a commit that referenced this pull request Nov 25, 2022
@rparrett
Warn instead of erroring when max_font_atlases is exceeded (#6673)
41149d6 
# Objective

Fixes #6642

In a way that doesn't create any breaking changes, as a possible way to fix the above in a patch release.

## Solution

Don't actually remove font atlases when `max_font_atlases` is exceeded. Add a warning instead.

Keep `TextError::ExceedMaxTextAtlases` and `TextSettings` as-is so we don't break anything.

This is a bit of a cop-out, but the problems revealed by #6642 seem very challenging to fix properly.

Maybe follow up later with something more like https://github.com/rparrett/bevy/commits/remove-max-font-atlases later, if this is the direction we want to go.

## Note

See previous attempt at a "simple fix" that only solved some of the issues: #6666
bors bot pushed a commit that referenced this pull request Nov 25, 2022
@rparrett
Warn instead of erroring when max_font_atlases is exceeded (#6673)
d1528df 
# Objective

Fixes #6642

In a way that doesn't create any breaking changes, as a possible way to fix the above in a patch release.

## Solution

Don't actually remove font atlases when `max_font_atlases` is exceeded. Add a warning instead.

Keep `TextError::ExceedMaxTextAtlases` and `TextSettings` as-is so we don't break anything.

This is a bit of a cop-out, but the problems revealed by #6642 seem very challenging to fix properly.

Maybe follow up later with something more like https://github.com/rparrett/bevy/commits/remove-max-font-atlases later, if this is the direction we want to go.

## Note

See previous attempt at a "simple fix" that only solved some of the issues: #6666
cart pushed a commit that referenced this pull request Nov 30, 2022
@rparrett @cart
Warn instead of erroring when max_font_atlases is exceeded (#6673)
6733102 
# Objective

Fixes #6642

In a way that doesn't create any breaking changes, as a possible way to fix the above in a patch release.

## Solution

Don't actually remove font atlases when `max_font_atlases` is exceeded. Add a warning instead.

Keep `TextError::ExceedMaxTextAtlases` and `TextSettings` as-is so we don't break anything.

This is a bit of a cop-out, but the problems revealed by #6642 seem very challenging to fix properly.

Maybe follow up later with something more like https://github.com/rparrett/bevy/commits/remove-max-font-atlases later, if this is the direction we want to go.

## Note

See previous attempt at a "simple fix" that only solved some of the issues: #6666
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@rparrett @ItsDoot
Warn instead of erroring when max_font_atlases is exceeded (bevyengin…
e3ff908 
…e#6673)

# Objective

Fixes bevyengine#6642

In a way that doesn't create any breaking changes, as a possible way to fix the above in a patch release.

## Solution

Don't actually remove font atlases when `max_font_atlases` is exceeded. Add a warning instead.

Keep `TextError::ExceedMaxTextAtlases` and `TextSettings` as-is so we don't break anything.

This is a bit of a cop-out, but the problems revealed by bevyengine#6642 seem very challenging to fix properly.

Maybe follow up later with something more like https://github.com/rparrett/bevy/commits/remove-max-font-atlases later, if this is the direction we want to go.

## Note

See previous attempt at a "simple fix" that only solved some of the issues: bevyengine#6666
@ickshonpe ickshonpe mentioned this pull request Feb 28, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior
Projects
None yet
Milestone
0.9.1
Development

Successfully merging this pull request may close these issues.

FontAtlasSet is broken
2 participants
@rparrett @alice-i-cecile