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

Implement growing logic for TextAtlas #34

Merged
merged 4 commits into from
Jul 4, 2023
Merged

Conversation

hecrj
Copy link
Contributor

@hecrj hecrj commented Mar 19, 2023

The TextAtlas will have an initial size of 256 (we could make this configurable) and try_allocate will keep track of the glyphs in use in the current frame, returning None when the LRU glyph is in use.

In that case, TextRenderer::prepare will return
PrepareError::AtlasFull with the ContentType of the atlas that is full. The user of the library can then call TextAtlas::grow with the provided ContentType to obtain a bigger atlas (by 256).

A TextAtlas::grow call clears the whole atlas and, as a result, all of the prepare calls need to be repeated in a frame until they all succeed. Overall, the atlas will rarely need to grow and so the calls will not need to be repated often.

Finally, the user needs to call TextAtlas::trim at the end of the frame. This allows us to clear the glyphs in use collection in the atlas. Maybe there is a better way to model this in an API that forces the user to trim the atlas (e.g. make trim return a new type and changing prepare and render to take that type instead).

@genusistimelord
Copy link
Contributor

genusistimelord commented Mar 19, 2023

I decided for my own Texture Storage to use Texture Layers With a Garbage Collector via 2 hash_maps. But after discussion this with Hecj I relies that the LRU + HashSet can be more efficient in the long run versus a Garbage like collector behind a Pressure threshhold. Edited this so others wont be confused...

@hecrj
Copy link
Contributor Author

hecrj commented Mar 19, 2023

@genusistimelord I don't understand.

  • The purpose of the LRU is to evict the oldest glyphs first when strictly necessary. There is no need to deallocate every unused glyph every frame.
  • You should not use collection lengths for pressure, since glyphs can have very different sizes.
  • We keep track of the glyphs used with the glyphs_in_use set. There is no need for 2 maps.
  • Unbounded shouldn't be a problem since the atlas itself has a defined bound (its size).
  • Layers do not solve the problem of growing the atlas dimensions. We would have to choose a base fixed size if we only rely on layers. Allocation and eviction also become more costly and complex.

@genusistimelord
Copy link
Contributor

genusistimelord commented Mar 20, 2023

Ok yeah After discussing this with you I agree but I do think we need Texture Atlas Layering to avoid any pitfalls as resizing a Texture Atlas to a new size really doesn't solves the issue and requires us to re-upload all the glyphs currently being used individually. So I would think it be more Efficient in the long run to have the ability to use Layers and make it so a new layer is only created when there is no room in the Atlas and no room can be made by de-allocating unused Glyphs.

@grovesNL
Copy link
Owner

Thank you for the PR!

Is there a way we could perform the growing automatically internally and copy the old atlas into the new one after resizing? If it's possible, I'd prefer that callers didn't need to re-prepare, although it seems ok for callers to request grow proactively if they know free space is running low.

I'd also like trim to be opt-in for the same reason (I see it similar to shrink_to_fit)

@hecrj
Copy link
Contributor Author

hecrj commented Mar 28, 2023

@grovesNL I couldn't find a way to resize an existing BucketedAtlasAllocator.

We could reallocate the glyphs, but they may have different positions and the UVs of the vertices computed before the resize will be invalid.

@grovesNL
Copy link
Owner

grovesNL commented Apr 1, 2023

Good point, opened nical/etagere#22

We also might be able to work around it in the meantime by internally remapping all of the old UVs to the new UVs, but that could be complicated (especially for clipped glyphs) without storing extra info during prepare.

@hecrj hecrj force-pushed the growing-atlas branch 2 times, most recently from 52d54e3 to c5540bf Compare April 8, 2023 05:32
The `TextAtlas` will have an initial size of `256` (we could make this
configurable) and `try_allocate` will keep track of the glyphs in use
in the current frame, returning `None` when the LRU glyph is in use.

In that case, `TextRenderer::prepare` will return
`PrepareError::AtlasFull` with the `ContentType` of the atlas that is
full. The user of the library can then call `TextAtlas::grow` with the
provided `ContentType` to obtain a bigger atlas (by `256`).

A `TextAtlas::grow` call clears the whole atlas and, as a result, all of
the `prepare` calls need to be repeated in a frame until they all
succeed. Overall, the atlas will rarely need to grow and so the calls
will not need to be repated often.

Finally, the user needs to call `TextAtlas::trim` at the end of the
frame. This allows us to clear the glyphs in use collection in the atlas. Maybe
there is a better way to model this in an API that forces the user to
trim the atlas (e.g. make `trim` return a new type and changing `prepare` and `render` to take that type instead).
@hecrj
Copy link
Contributor Author

hecrj commented Jun 15, 2023

I have rebased on top of main and also implemented automatic re-upload of the glyphs when growing the atlas in 65276a1, which relies on nical/etagere#24.

Copy link
Owner

@grovesNL grovesNL left a comment

Choose a reason for hiding this comment

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

Thank you!

src/text_atlas.rs Show resolved Hide resolved
src/text_atlas.rs Show resolved Hide resolved
@hecrj
Copy link
Contributor Author

hecrj commented Jul 4, 2023

nical/etagere#24 was finally merged, which should unblock this! I have updated etagere and hopefully fixed any outstanding issues!

Copy link
Owner

@grovesNL grovesNL left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

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

Successfully merging this pull request may close these issues.

3 participants