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

Texture atlas preprocessing & the art production pipeline. #13713

Open
brandon-reinhart opened this issue Jun 6, 2024 · 4 comments
Open

Texture atlas preprocessing & the art production pipeline. #13713

brandon-reinhart opened this issue Jun 6, 2024 · 4 comments
Labels
A-Assets Load files from disk to use for things like images, models, and sounds A-Editor Graphical tools to make Bevy games A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible S-Needs-Design This issue requires design work to think about how it would best be accomplished

Comments

@brandon-reinhart
Copy link
Contributor

In order to make Bevy a tier 1 engine for 2D games, a better texture atlas art production pipeline is needed.

Requirements:

  • It should be possible to drop texture atlas source art into a directory and have the asset preprocessor rebuild the atlas.
  • In this sense, the individual source art is uncooked and the atlas is a cooked (shippable) asset.
  • More texture atlas services should be exposed through this preprocessing (and the underlying API).
  • Preprocessing should be resilient to bad data, communicating the nature of the error without panics. Cooking content should be a viable runtime development activity by non-programmers on the team.

Additional Features:

  • The atlas preprocessor should be configurable.
  • Option: Trim whitespace. To pack efficiently, the asset preprocessor should be able to trim whitespace and record in metadata the amount trimmed so that the original UVs can be reconstructed.
  • Option: Rotate images. To pack efficiently, the asset preprocessor should be able to rotate images 90deg and record in the metadata the rotation bit so that the original UVs can be reconstructed.
  • Option: Paginated atlases. It should be possible to specify the max texture size of an atlas and, if the source art exceeds this arrangement, produce additional atlas pages. Meta data would track this and use the appropriate texture binding at render time.
  • Option: Resampling rules. It may be useful to be able to specify resampling rules for input images before they are written to the atlas.

Many of these are features of the Esoteric atlas packer, which is very good.

@brandon-reinhart brandon-reinhart added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jun 6, 2024
@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds A-Editor Graphical tools to make Bevy games S-Needs-Design This issue requires design work to think about how it would best be accomplished A-Rendering Drawing game state to the screen and removed S-Needs-Triage This issue needs to be labelled labels Jun 6, 2024
@brandon-reinhart
Copy link
Contributor Author

Attacking this work probably breaks down into these steps.

  1. Research what needs to be done for "N-to-one" asset processing in assetv2. This means that a single output file is produced as a result of any file in a tracked directory changing (modify/add/delete). This may require a PR to file down any rough edges.

  2. Get preprocessing and loading working with basic bevy texture atlases using existing features and minimal or no metadata. So we can modify / add / delete and get a functional bevy texture atlas on the other end.

  3. Add the advanced configuration features once the simple model is in place and working. (probably a PR per feature)

@msvbg
Copy link
Contributor

msvbg commented Jun 8, 2024

Related PR/issue: #11873 / #11634

@clarfonthey
Copy link
Contributor

Found my way at this issue and wanted to just comment on a few low-hanging fruit I noticed.

First, TextureAtlasLayout includes a map from the atlas back to the original images, but this feels like a mistake. By itself, TextureAtlasLayout is a very simple structure that could be made de/serializable, but having asset IDs embedded directly inside it complicates things. IMHO, this should be instead delegated to another type, TextureAtlasSources, which is generated when an atlas is generated, but isn't needed if the atlas is loaded directly.

Second, while the TextureAtlasBuilder is nice, it feels like the majority of the settings for packing a texture atlas should be located in a struct which can also be de/serialized, so it can be used for the input to an asset processor.

Third, there could probably easily be a dedicated asset processor for texture atlases (there aren't any at the moment, but this could be the first!) based upon the above two types.

Finally, it's very weird that the type TextureAtlas doesn't refer to the entire atlas, but a texture on the atlas. It would make sense to have a TextureAtlas type storing the actual atlas asset (including the image), and an AtlasTexture to store the reference to the specific asset and an index.

Would be more than happy to implement a couple of these, after I look more into the code. Just figured I'd star the discussion before I do, since I think it's worth making sure I didn't misread anything.

@msvbg
Copy link
Contributor

msvbg commented Sep 21, 2024

@clarfonthey I have largely worked around Bevy’s texture atlases to get something usable for my game. My current workflow is custom but pretty good! I agree with your raised issues, and there are even more problems. It might even warrant its own working group.

github-merge-queue bot pushed a commit that referenced this issue Sep 23, 2024
# Objective

In order to derive `Reflect`, all of a struct's fields must implement
`FromReflect`. [As part of looking into some of the work mentioned
here](#13713 (comment)),
I noticed that `TextureFormat` doesn't implement `Reflect`, and decided
to split that into a separate PR.

## Solution

I decided that `TextureFormat` should be a `reflect_value` since,
although one variant has fields, most users will treat this as an opaque
value set explicitly. It also substantially reduces the complexity of
the implementation.

For now, this implementation isn't actually used by any crates, so, I
decided to not preemptively enable the feature on anything. But it's
technically an option, now, and more `wgpu` types can be added in the
future.

## Testing

Everything compiles okay, and I can't really see how this could be done
incorrectly given the above constraints.
github-merge-queue bot pushed a commit that referenced this issue 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 issue 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.
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-Editor Graphical tools to make Bevy games A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible S-Needs-Design This issue requires design work to think about how it would best be accomplished
Projects
None yet
Development

No branches or pull requests

4 participants