Skip to content

Add TextureAtlasLoader#11873

Open
lynn-lumen wants to merge 15 commits intobevyengine:mainfrom
lynn-lumen:texture_atlas_loader
Open

Add TextureAtlasLoader#11873
lynn-lumen wants to merge 15 commits intobevyengine:mainfrom
lynn-lumen:texture_atlas_loader

Conversation

@lynn-lumen
Copy link
Copy Markdown
Contributor

@lynn-lumen lynn-lumen commented Feb 15, 2024

Objective

Solution

  • Adding an AssetLoader for TextureAtlasLayout The loader has three extensions:
    • atlas.ron: Creates a TextureAtlasLayout with the specified size and texture rectangles.
    • atlas.grid.ron: Creates a TextureAtlasLayout with the specified grid, similar to TextureAtlasLayout::from_grid.
    • atlas.composed.ron: Creates a TextureAtlasLayout from the specified source textures by composing them using TextureAtlasBuilder. The composed image returned by the TextureAtlasBuilder is added as a labeled asset: composed_texture .

The AssetLoader implementation is based on @viridia's code as posted in the original issue

@lynn-lumen lynn-lumen changed the title Add TextureAtlasLayoutLoader Add TextureAtlasLoader Feb 15, 2024
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen A-Assets Load files from disk to use for things like images, models, and sounds labels Feb 15, 2024
@viridia
Copy link
Copy Markdown
Contributor

viridia commented Feb 16, 2024

I think this would go a lot better if there was an example showing how to use it. For example, imagine a novice user who wants to spawn an AtlasImageBundle. That accepts a TextureAtlas, not a TextureAtlasLayout, but it won't be obvious to the novice how to get one from the other (it's not obvious to me either).

My original PR assumed that you could spawn an AtlasImageBundle in just two steps:

  • Load a TextureAtlas resource and get a handle. This would automatically load the image as a dependency of the atlas, since the texture atlas resource would include a relative path to the image.
  • Spawn an AtlasImageBundle using the returned handle.

However, that no longer works due to the changes in the way texture atlases work.

@lynn-lumen
Copy link
Copy Markdown
Contributor Author

I think this would go a lot better if there was an example showing how to use it. For example, imagine a novice user who wants to spawn an AtlasImageBundle. That accepts a TextureAtlas, not a TextureAtlasLayout, but it won't be obvious to the novice how to get one from the other (it's not obvious to me either).

There already are some examples demonstrating how to use TextureAtlas and TextureAtlasLayout. However I agree, that an example demonstrating how to load a TextureAtlasLayout is useful.

I have added an example like that now, but I would appreciate someone reading over that since my English is not that good.

@github-actions
Copy link
Copy Markdown
Contributor

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@lynn-lumen lynn-lumen marked this pull request as ready for review February 16, 2024 13:39
@viridia
Copy link
Copy Markdown
Contributor

viridia commented Feb 17, 2024

Looks like there are some problems with the dependencies. Otherwise looks good.

@lynn-lumen
Copy link
Copy Markdown
Contributor Author

lynn-lumen commented Feb 17, 2024

Looks like there are some problems with the dependencies.

This is apparently an issue on main. The test will get triggered anytime you modify a Cargo.toml and failing should be non-blocking for merging. I asked on discord because I was confused 😅

There's also a related issue: #11917

Comment on lines +4 to +7
(min: (0, 8), max: (16, 24)),
(min: (16, 8), max: (32, 24)),
(min: (32, 8), max: (48, 24)),
(min: (48, 8), max: (64, 24)),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Having a name for each packed sprite here might be useful too.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nevermind, this is out of scope for this PR.

github-merge-queue bot pushed a commit that referenced this pull request Sep 30, 2024
…tureAtlasLayout` serializable (#15344)

# Objective

Mostly covers the first point in
#13713 (comment)

The idea here is that a lot of people want to load their own texture
atlases, and many of them do this by deserializing some custom version
of `TextureAtlasLayout`. This makes that a little easier by providing
`serde` impls for them.

## Solution

In order to make `TextureAtlasLayout` serializable, the custom texture
mappings that are added by `TextureAtlasBuilder` were separated into
their own type, `TextureAtlasSources`. The inner fields are made public
so people can create their own version of this type, although because it
embeds asset IDs, it's not as easily serializable. In particular,
atlases that are loaded directly (e.g. sprite sheets) will not have a
copy of this map, and so, don't need to construct it at all.

As an aside, since this is the very first thing in `bevy_sprite` with
`serde` impls, I've added a `serialize` feature to the crate and made
sure it gets activated when the `serialize` feature is enabled on the
parent `bevy` crate.

## Testing

I was kind of shocked that there isn't anywhere in the code besides a
single example that actually used this functionality, so, it was
relatively straightforward to do.

In #13713, among other places, folks have mentioned adding custom
serialization into their pipelines. It would be nice to hear from people
whether this change matches what they're doing in their code, and if
it's relatively seamless to adapt to. I suspect that the answer is yes,
but, that's mainly the only other kind of testing that can be added.

## Migration Guide

`TextureAtlasBuilder` no longer stores a mapping back to the original
images in `TextureAtlasLayout`; that functionality has been added to a
new struct, `TextureAtlasSources`, instead. This also means that the
signature for `TextureAtlasBuilder::finish` has changed, meaning that
calls of the form:

```rust
let (atlas_layout, image) = builder.build()?;
```

Will now change to the form:

```rust
let (atlas_layout, atlas_sources, image) = builder.build()?;
```

And instead of performing a reverse-lookup from the layout, like so:

```rust
let atlas_layout_handle = texture_atlases.add(atlas_layout.clone());
let index = atlas_layout.get_texture_index(&my_handle);
let handle = TextureAtlas {
    layout: atlas_layout_handle,
    index,
};
```

You can perform the lookup from the sources instead:

```rust
let atlas_layout = texture_atlases.add(atlas_layout);
let index = atlas_sources.get_texture_index(&my_handle);
let handle = TextureAtlas {
    layout: atlas_layout,
    index,
};
```

Additionally, `TextureAtlasSources` also has a convenience method,
`handle`, which directly combines the index and an existing
`TextureAtlasLayout` handle into a new `TextureAtlas`:

```rust
let atlas_layout = texture_atlases.add(atlas_layout);
let handle = atlas_sources.handle(atlas_layout, &my_handle);
```

## Extra notes

In the future, it might make sense to combine the three types returned
by `TextureAtlasBuilder` into their own struct, just so that people
don't need to assign variable names to all three parts. In particular,
when creating a version that can be loaded directly (like #11873), we
could probably use this new type.
robtfm pushed a commit to robtfm/bevy that referenced this pull request Oct 4, 2024
…tureAtlasLayout` serializable (bevyengine#15344)

# Objective

Mostly covers the first point in
bevyengine#13713 (comment)

The idea here is that a lot of people want to load their own texture
atlases, and many of them do this by deserializing some custom version
of `TextureAtlasLayout`. This makes that a little easier by providing
`serde` impls for them.

## Solution

In order to make `TextureAtlasLayout` serializable, the custom texture
mappings that are added by `TextureAtlasBuilder` were separated into
their own type, `TextureAtlasSources`. The inner fields are made public
so people can create their own version of this type, although because it
embeds asset IDs, it's not as easily serializable. In particular,
atlases that are loaded directly (e.g. sprite sheets) will not have a
copy of this map, and so, don't need to construct it at all.

As an aside, since this is the very first thing in `bevy_sprite` with
`serde` impls, I've added a `serialize` feature to the crate and made
sure it gets activated when the `serialize` feature is enabled on the
parent `bevy` crate.

## Testing

I was kind of shocked that there isn't anywhere in the code besides a
single example that actually used this functionality, so, it was
relatively straightforward to do.

In bevyengine#13713, among other places, folks have mentioned adding custom
serialization into their pipelines. It would be nice to hear from people
whether this change matches what they're doing in their code, and if
it's relatively seamless to adapt to. I suspect that the answer is yes,
but, that's mainly the only other kind of testing that can be added.

## Migration Guide

`TextureAtlasBuilder` no longer stores a mapping back to the original
images in `TextureAtlasLayout`; that functionality has been added to a
new struct, `TextureAtlasSources`, instead. This also means that the
signature for `TextureAtlasBuilder::finish` has changed, meaning that
calls of the form:

```rust
let (atlas_layout, image) = builder.build()?;
```

Will now change to the form:

```rust
let (atlas_layout, atlas_sources, image) = builder.build()?;
```

And instead of performing a reverse-lookup from the layout, like so:

```rust
let atlas_layout_handle = texture_atlases.add(atlas_layout.clone());
let index = atlas_layout.get_texture_index(&my_handle);
let handle = TextureAtlas {
    layout: atlas_layout_handle,
    index,
};
```

You can perform the lookup from the sources instead:

```rust
let atlas_layout = texture_atlases.add(atlas_layout);
let index = atlas_sources.get_texture_index(&my_handle);
let handle = TextureAtlas {
    layout: atlas_layout,
    index,
};
```

Additionally, `TextureAtlasSources` also has a convenience method,
`handle`, which directly combines the index and an existing
`TextureAtlasLayout` handle into a new `TextureAtlas`:

```rust
let atlas_layout = texture_atlases.add(atlas_layout);
let handle = atlas_sources.handle(atlas_layout, &my_handle);
```

## Extra notes

In the future, it might make sense to combine the three types returned
by `TextureAtlasBuilder` into their own struct, just so that people
don't need to assign variable names to all three parts. In particular,
when creating a version that can be loaded directly (like bevyengine#11873), we
could probably use this new type.
@janhohenheim
Copy link
Copy Markdown
Member

Triage: has merge conflicts
@lynn-lumen do you want to update this PR or should I tag it S-Needs-Adoption? :)

@janhohenheim janhohenheim added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label May 17, 2025
@lynn-lumen
Copy link
Copy Markdown
Contributor Author

lynn-lumen commented May 17, 2025

Please tag it as S-Needs-Adoption. I can't find it anymore, but I believe there was a discussion on discord regarding whether this was the correct way forward, I could be wrong though.

@janhohenheim janhohenheim added S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels May 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Assets Load files from disk to use for things like images, models, and sounds A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible S-Adopt-Me The original PR author has no intent to complete this work. Pick me up!

Projects

No open projects
Status: No status

Development

Successfully merging this pull request may close these issues.

AssetLoader for TextureAtlas

5 participants