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

Add Support for Dynamic Asset Collections #198

Merged

Conversation

lgrossi
Copy link
Contributor

@lgrossi lgrossi commented Feb 25, 2024

This PR introduces the ability to load and build collections of dynamic assets in bevy_asset_loader, addressing the need outlined in issue #186. The enhancement allows for more flexible asset management, enabling users to define and group dynamic assets in structured collections.

Motivation

Previously, loading multiple dynamic assets required individual handling for each asset, complicating the asset loading process and cluttering the codebase. With this update, users can now group related assets together, simplifying the loading process (specially for applications that handles a huge number of assets dynamically).

Changes

DynamicAsset Trait for Vec: Implemented the DynamicAsset trait for Vec, enabling the loading and building of asset collections.

StandardDynamicAssetArrayCollection: Introduced a new struct to represent collections of dynamic assets, allowing assets to be grouped and registered under a common key.

Usage Example

With these changes, users can now define array of asset collections in RON format:

({
    "layouts": [
        TextureAtlasLayout (
            tile_size_x: 32.,
            tile_size_y: 32.,
            columns: 12,
            rows: 12,
        ),
        TextureAtlasLayout (
            tile_size_x: 32.,
            tile_size_y: 64.,
            columns: 12,
            rows: 6,
        ),
        TextureAtlasLayout (
            tile_size_x: 64.,
            tile_size_y: 32.,
            columns: 6,
            rows: 12,
        ),
        TextureAtlasLayout (
            tile_size_x: 64.,
            tile_size_y: 64.,
            columns: 6,
            rows: 6,
        ),
    ],
    "mixed": [
        StandardMaterial (
            path: "images/tree.png",
        ),
        Image (
            path: "ryot_mascot.png",
            sampler: Nearest
        ),
        Image (
            path: "ryot_mascot.png",
            sampler: Nearest
        ),
    ],
})

And the rust code would look like:

    #[asset(key = "layouts", collection(typed))]
    atlas_layout: Vec<Handle<TextureAtlasLayout>>,

    #[asset(key = "mixed", collection)]
    mixed_handlers: Vec<UntypedHandle>,

Conclusion

This update offers a straightforward method for managing multiple dynamic assets within the bevy_asset_loader. It's particularly beneficial for applications dealing with a large number of assets dynamically, aiming to simplify the process and improve developer experience. By grouping assets into collections, it hopes to make dynamic asset handling more accessible and less cumbersome.

@NiklasEi
Copy link
Owner

Thanks for offering your solution 🙂
#151 would allow deserializing collections of dynamic assets without a new DynamicAssetCollection implementation but it has been blocked on the next ron release for a while. For this solution, users will have to register this new dynamic asset collection type with the loading state and cannot mix collections and single dynamic assets in the same file.
I would be open to merging this as a temporary feature until we can get #151 as the more general solution to the same issue. What do you think?

@lgrossi
Copy link
Contributor Author

lgrossi commented Feb 25, 2024

Hey, indeed #151 seems like a more general solution. Curious to know the reasons behind opting for not allowing single collections and multi collections in the same schema.

I'm happy with this being a temp solution til we reach there, but I'm also ok with this not being merged (I can use my temp branch til we got #151 merged). I was just aiming to share this since it's quite a effortless/clean work around for the time being - also, it's cleaner than doing it downstream, since that in a downstream application using your lib one would need to create a wrapper struct just for that, instead of implementing DynamicAsset to Vec<_> directly.

Thanks for the inputs. 😄

@NiklasEi
Copy link
Owner

Curious to know the reasons behind opting for not allowing single collections and multi collections in the same schema.

I meant this PR doesn't allow it, because a single file will be loaded as StandardDynamicAssetArrayCollection or as StandardDynamicAssetCollection. #151 allows for mixing single assets and collections in the same file.

@lgrossi lgrossi force-pushed the lucas/set-of-dynamic-standard-assets branch 4 times, most recently from 531d626 to d9b09db Compare March 8, 2024 18:42
@lgrossi
Copy link
Contributor Author

lgrossi commented Mar 8, 2024

@NiklasEi I finally had some time to add documentation and tests. Please let me know if you want me to add anything else before merging this PR.

@lgrossi lgrossi force-pushed the lucas/set-of-dynamic-standard-assets branch 4 times, most recently from f840ae7 to 4fb0965 Compare March 10, 2024 18:20
@lgrossi
Copy link
Contributor Author

lgrossi commented Mar 10, 2024

@NiklasEi I finally had some time to add documentation and tests. Please let me know if you want me to add anything else before merging this PR.

Ok, there was some flakiness in the test due to the nature of RON serialization/deserialization (it doesn't guarantee the order of the elements). I changed it to compare before/after deserialized objects instead of the RON strings, it should be fine now.

@lgrossi lgrossi force-pushed the lucas/set-of-dynamic-standard-assets branch from 4fb0965 to 9888663 Compare March 11, 2024 16:30
@lgrossi
Copy link
Contributor Author

lgrossi commented Mar 20, 2024

Hi @NiklasEi is there anything else I can do to help getting this merged? Let me know if I can help 😄

@NiklasEi NiklasEi merged commit fce89e2 into NiklasEi:main Mar 20, 2024
13 checks passed
@NiklasEi
Copy link
Owner

Thank you 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants