Allow TextureAtlasBuilder in AssetLoader #11548
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
matiqo15
added
A-Assets
|
@doonv, want to take a look at this one too? And lemme know if you'd like org membership so you can triage things/I can request your review directly. |
|
@B-Reif @adtennant I'd be interested in your opinions too. |
doonv
reviewed
Jan 27, 2024
Comment on lines
31
to
32
| /// Collection of textures and their size to be packed into an atlas | ||
| textures_to_place: Vec<(AssetId<Image>, Extent3d)>, | ||
| textures_to_place: Vec<(Option<AssetId<Image>>, &'a Image)>, |
There was a problem hiding this comment.
This doc comment should be updated
doonv
approved these changes
Jan 27, 2024
alice-i-cecile
approved these changes
Jan 27, 2024
alice-i-cecile
added
the
S-Ready-For-Final-Review
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
Allow TextureAtlasBuilder in AssetLoader.
Fixes #2987
Solution
finish, but &Image instead.Issues
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
add_texturecan be called without providing an AssetIdfinishno longer takes AssetsMigration Guide
add_textureyou need to wrap your AssetId in Somefinishnow returns the atlas texture image directly instead of a handle. Provide the atlas texture toaddon Assets to get a Handle