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

Support sharing Pipeline state between TextAtlas #95

Merged
merged 5 commits into from
May 8, 2024

Conversation

hecrj
Copy link
Sponsor Contributor

@hecrj hecrj commented May 8, 2024

Creating a new TextAtlas is quite expensive now since it will recompile shaders and recreate pipelines.

This PR splits the pipeline cache into its own Pipeline type that can be cheaply cloned and reused by different TextAtlas instances.

@grovesNL
Copy link
Owner

grovesNL commented May 8, 2024

Thank you! What's the reason you'd like to use separate text atlases vs. reusing one atlas? The existing API was designed around reusing few atlases as possible, so I thought the pipeline recreation cost would be alright in that case.

Is there a nice way we could do this without exposing Pipeline as part of the public API? e.g., internally sharing a lazily initialized OnceCell. I'm just thinking about people wondering why they have to manage glyphon's pipeline, although maybe it's just a naming thing and we could call it "cache"/"shared"/something else and still feel intuitive. Later on we could probably consider moving some of the other shared types like FontSystem/SwashCache in there too since they'll mostly be used as singletons.

src/pipeline.rs Outdated Show resolved Hide resolved
@hecrj
Copy link
Sponsor Contributor Author

hecrj commented May 8, 2024

What's the reason you'd like to use separate text atlases vs. reusing one atlas?

The issue with reusing a single atlas is that you have to prepare all the text every frame, or you risk having missing glyphs or artifacts. However, sometimes you have a bunch of text that doesn't change that often; or that may not be presented at the moment but may appear later again.

By having a local atlas for these sections of text, you can simply skip prepare during a frame and call render directly—effectively caching everything. Not sure if there is an effective way to achieve this with a single atlas.

In iced, I currently use a local atlas for every canvas::Cache; which is a data structure that should contain primitives that don't change that much.

Is there a nice way we could do this without exposing Pipeline as part of the public API? e.g., internally sharing a lazily initialized OnceCell

Sure! We could have a once cell singleton static with the Pipeline since its whole API is immutable. I didn't do that right away because it basically makes dropping that memory impossible; but I can't think of a scenario where that would be an issue of the top of my head.

@grovesNL
Copy link
Owner

grovesNL commented May 8, 2024

Thanks for the explanation, that makes sense to me. Maybe we could start with the singleton and split it out into a separate Pipeline type if anyone needs to be able to free it (we could even add an explicit destroy somewhere for that case later if we didn't want to split it out).

@hecrj
Copy link
Sponsor Contributor Author

hecrj commented May 8, 2024

@grovesNL Gotcha! 4112732 makes Pipeline private by leveraging a static OnceCell.

Cargo.toml Outdated Show resolved Hide resolved
@hecrj
Copy link
Sponsor Contributor Author

hecrj commented May 8, 2024

Hmm... A OnceCell introduces issues because glyphon may be used by a different Device in the same execution. Just noticed the problem when benchmarking iced.

@grovesNL
Copy link
Owner

grovesNL commented May 8, 2024

Alright that's too bad. Maybe we could go back to the original approach but name it glyphon::Cache or something, and plan to use it to share some of the other singleton-ish types later too.

@hecrj
Copy link
Sponsor Contributor Author

hecrj commented May 8, 2024

@grovesNL Done that in 08d218c.

I think the breakage should be minimal for most users, since the only thing asking for a Cache is TextAtlas::new.

@grovesNL
Copy link
Owner

grovesNL commented May 8, 2024

Thank you!

@grovesNL grovesNL merged commit 5aed9e1 into grovesNL:main May 8, 2024
1 check passed
@hecrj hecrj deleted the shared-pipeline-state branch May 9, 2024 09:52
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.

None yet

2 participants