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

Rework TextureAtlasBuilder APIs for asset loaders #2987

Closed
B-Reif opened this issue Oct 17, 2021 · 1 comment · Fixed by #11548
Closed

Rework TextureAtlasBuilder APIs for asset loaders #2987

B-Reif opened this issue Oct 17, 2021 · 1 comment · Fixed by #11548
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use

Comments

@B-Reif
Copy link
Contributor

B-Reif commented Oct 17, 2021

What problem does this solve or what need does it fill?

Currently the TextureAtlasBuilder API requires the use of the Assets API, tracking its constituent textures through Handles and Assets<Texture>. This limits atlas construction to contexts where the Assets API is available. Specifically, it would help to be able to construct an Atlas during Asset::load where only the load_context API is available.

What solution would you like?

Expand or modify the TextureAtlasBuilder API to work with lower-level types or with a load context.

What alternative(s) have you considered?

Currently the only options are to

  • move atlas construction into Systems, which isn't ideal for conveniently loading Asset types, or
  • re-implement the TextureAtlasBuilder internals externally
@B-Reif B-Reif added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Oct 17, 2021
@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled C-Feature A new feature, making something new possible labels Oct 18, 2021
@adtennant
Copy link
Contributor

#8633 currently prevents "re-implement the TextureAtlasBuilder internals externally".

@adtennant adtennant mentioned this issue May 21, 2023
Merged
@KirmesBude KirmesBude mentioned this issue Jan 26, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Jan 27, 2024
@KirmesBude
Allow TextureAtlasBuilder in AssetLoader (#11548)
3851679 
# 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 issue Feb 6, 2024
@KirmesBude @tjamaan
Allow TextureAtlasBuilder in AssetLoader (bevyengine#11548)
40044f6 
# 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>
@s-puig s-puig mentioned this issue Sep 22, 2024
Merged
@alice-i-cecile alice-i-cecile mentioned this issue Sep 22, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow TextureAtlasBuilder in AssetLoader KirmesBude/bevy
3 participants
@adtennant @alice-i-cecile @B-Reif