TextureAtlasBuilder now respects insertion order #11474
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KirmesBude
force-pushed
the
feature/texture_atlas_builder_order
branch
from
6846b84 to
05c5478
Compare
doonv
approved these changes
Jan 22, 2024
alice-i-cecile
added
C-Bug
alice-i-cecile
approved these changes
Jan 22, 2024
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
@alice-i-cecile There is actually one subtle change that I did not think about. If you add the same image multiple times to the TextureAtlasBuilder it would only be added once to the TextureAtlas. Now it will appear multiple times (the atlas texture itself will still contain the image data only once, but there are additional indices that will point to the same part of the atlas texture). |
|
Hmm. I'm really not sure if that's desired behavior or not. |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 27, 2024
# Objective Allow TextureAtlasBuilder in AssetLoader. Fixes #2987 ## Solution - TextureAtlasBuilder no longer hold just AssetIds that are used to retrieve the actual image data in `finish`, but &Image instead. - TextureAtlasBuilder now required AssetId only optionally (and it is only used to retrieve the index from the AssetId in TextureAtlasLayout), ## Issues - The issue mentioned here #11474 (comment) now also extends to the actual atlas texture. In short: Calling add_texture multiple times for the same texture will lead to duplicate image data in the atlas texture and additional indices. If you provide an AssetId we can probably do something to de-duplicate the entries while keeping insertion order (suggestions welcome on how exactly). But if you don't then we are out of luck (unless we can and want to hash the image, which I do not think we want). --- ## Changelog ### Changed - TextureAtlasBuilder `add_texture` can be called without providing an AssetId - TextureAtlasBuilder `finish` no longer takes Assets<Image> and no longer returns a Handle<Image> ## Migration Guide - For `add_texture` you need to wrap your AssetId in Some - `finish` now returns the atlas texture image directly instead of a handle. Provide the atlas texture to `add` on Assets<Texture> to get a Handle<Image>
tjamaan
pushed a commit
to tjamaan/bevy
that referenced
this pull request
Feb 6, 2024
# Objective Allow TextureAtlasBuilder in AssetLoader. Fixes bevyengine#2987 ## Solution - TextureAtlasBuilder no longer hold just AssetIds that are used to retrieve the actual image data in `finish`, but &Image instead. - TextureAtlasBuilder now required AssetId only optionally (and it is only used to retrieve the index from the AssetId in TextureAtlasLayout), ## Issues - The issue mentioned here bevyengine#11474 (comment) now also extends to the actual atlas texture. In short: Calling add_texture multiple times for the same texture will lead to duplicate image data in the atlas texture and additional indices. If you provide an AssetId we can probably do something to de-duplicate the entries while keeping insertion order (suggestions welcome on how exactly). But if you don't then we are out of luck (unless we can and want to hash the image, which I do not think we want). --- ## Changelog ### Changed - TextureAtlasBuilder `add_texture` can be called without providing an AssetId - TextureAtlasBuilder `finish` no longer takes Assets<Image> and no longer returns a Handle<Image> ## Migration Guide - For `add_texture` you need to wrap your AssetId in Some - `finish` now returns the atlas texture image directly instead of a handle. Provide the atlas texture to `add` on Assets<Texture> to get a Handle<Image>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
TextureAtlases are commonly used to drive animations described as a consecutive range of indices. The current TextureAtlasBuilder uses the AssetId of the image to determine the index of the texture in the TextureAtlas. The AssetId of an Image Asset can change between runs.
The TextureAtlas exposes
get_texture_indexto get the index from a given AssetId, but this needlessly complicates the process of creating a simple TextureAtlas animation.Fixes #2459
Solution