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 a method on AssetServer to create an asset from an existing one #1665

Closed
wants to merge 15 commits into from

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Mar 16, 2021

Some assets loaded from disk by the asset server have extra fields that are not possible to setup on load, as the asset loader is picking the values. Changing those values is quite bothersome as it requires the user to:

  • save the asset handle in a resource
  • add a system that will wait for the asset to be loaded then modify it

This PR add a method on AssetServer to simplify that:

// load the texture from disk
let texture_handle = asset_server.load("branding/icon.png");

// new texture with one pixel every two removed
let texture_handle_1 =
    asset_server.create_from(texture_handle, |texture: &Texture| {
        let new_data = texture
            .data
            .iter()
            .enumerate()
            .map(|(i, v)| {
                if i / texture.format.pixel_size() % 2 == 0 {
                    0
                } else {
                    *v
                }
            })
            .collect();
        Some(Texture {
            data: new_data,
            ..*texture
        })
    });

screenshot from example
Screenshot 2021-03-16 at 01 11 01

@alice-i-cecile alice-i-cecile added the A-Assets Load files from disk to use for things like images, models, and sounds label Mar 16, 2021
@mockersf
Copy link
Member Author

mockersf commented Mar 16, 2021

could this get considered for 0.5? I have a new (but very small, ~50 LoC) feature depending on it that I would love to see in the release to help build assets very easily from a ron file

@alice-i-cecile
Copy link
Member

could this get considered for 0.5? I have a new (but very small, ~50 LoC) feature depending on it that I would love to see in the release

(ping @cart on the above)

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Seems like a useful piece of functionality, especially when extended with more tools.

This will be a solid building block for real asset-transformation pipelines.

asset
.downcast_ref::<FROM>()
// this downcast can't fail as we know the actual types here
.expect("Error converting an asset to it's type, please open an issue in Bevy GitHub repository"),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.expect("Error converting an asset to it's type, please open an issue in Bevy GitHub repository"),
.expect("Error converting an asset to its type, please open an issue in Bevy GitHub repository"),

Copy link
Member

Choose a reason for hiding this comment

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

Is there no end-user workaround for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

it could be logged and ignored instead of expect, but this one actually can't happen as it's the one place where we know the actual types and they are constrained by the function signature

crates/bevy_asset/src/asset_server.rs Outdated Show resolved Hide resolved
examples/asset/asset_transformation.rs Outdated Show resolved Hide resolved
examples/asset/asset_transformation.rs Outdated Show resolved Hide resolved
@mockersf
Copy link
Member Author

mockersf commented Mar 17, 2021

I opened #1673 as an example of something that can be build on this

Copy link
Member

@alec-deason alec-deason left a comment

Choose a reason for hiding this comment

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

This is a super useful tool. I have a couple of style nits which you should feel free to ignore if they don't make sense to you.

crates/bevy_asset/src/asset_server.rs Outdated Show resolved Hide resolved
AssetLifecycleChannel { sender, receiver }
let (to_system, from_asset_server) = crossbeam_channel::unbounded();
AssetLifecycleChannel {
to_system,
Copy link
Member

Choose a reason for hiding this comment

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

When reading through this I stumbled on the name to_system several times because it seems like it must be connected to the IntoSystem but that doesn't make sense in context. I think I actually prefer the original sender and receiver, they're slightly more cryptic but they're conventional which makes them instantly recognizable.

Copy link
Member Author

@mockersf mockersf Mar 19, 2021

Choose a reason for hiding this comment

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

I disagree on that, but I guess it's a taste/habit situation. Plus I don't like having a variable named after its type.

I'm open to changing it back if others feel the same way

Copy link
Member

Choose a reason for hiding this comment

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

Your reason for making the change is good, I don't object to keeping it.

@alice-i-cecile
Copy link
Member

Closes #1833.

@cart
Copy link
Member

cart commented Apr 12, 2021

I think this might be better solved by #1393 (or some other async implementation). I think that would enable defining a system that (1) awaits an async load of the initial asset (2) yields control until the asset is finished loading (3) runs the post processing logic after the system has run.

There is some nuance, such as needing to call AssetServer::update_asset_storage() manually after the load finishes, but this might be solveable in a nice way.

I'd largely just like to try to build this out of existing/planned pieces (systems, events, asset server, etc) instead of adding new ones, if at all possible. This is a "dataflow" problem, which is a class of problem we've spend a lot of time solving in Bevy already.

@inodentry
Copy link
Contributor

This PR feels very similar to my old API proposal in #1307, in that both of these things are attempts to hook functionality into the asset system to run when an asset loads.

If async systems are the new suggested way to go about this sort of stuff, I guess they could address all of these scenarios and would resolve that old discussion, too?

@mockersf
Copy link
Member Author

mockersf commented Apr 12, 2021

(1) awaits an async load of the initial asset (2) yields control until the asset is finished loading (3) runs the post processing logic after the system has run.

This is already possible without async systems, having a system that wait for an asset to be loaded then transform it. It's more an ergonomic question:

  • A system can't be planned at start to run specifically for an asset handle that may be created during run, one would have to add a resource, something like ListOfAssetsToTransform and watch it in the system created at start.
  • Listening for an asset to be loaded then reacting to do something else is something that is not obvious.

This PR doesn't really do anything async, it just wraps that in a neat little function.

I'd largely just like to try to build this out of existing/planned pieces (systems, events, asset server, etc) instead of adding new ones, if at all possible. This is a "dataflow" problem, which is a class of problem we've spend a lot of time solving in Bevy already.

I would argue that this reuse completely the asset server and the system added when adding an asset type. The only new thing here is a variant of an enum that can be sent through an existing channel

@cart
Copy link
Member

cart commented Apr 14, 2021

My only major concern with the current implementation is that it adds a new api for registering "app logic" that is specific to AssetServer. If we can do this inside the ECS, we get the added flexibility of ECS / full access to the data encoded there.

I do acknowledge that from a different perspective this isn't "app logic" but rather "asset logic". Back when I was investigating evolving bevy_assets vs adopting Distill, I added the concept of "derived assets", which I used to enable things like "optimized textures derived from source textures". I think theres a world where we could integrate those changes and make them usable for these types of things. But if we go that route, I think we would need to reconsider Distill adoption as we'd be starting to encroach on that territory.

@mockersf
Copy link
Member Author

This would probably work completely differently/not be needed in a post Distill world 👍

@mockersf mockersf marked this pull request as draft April 23, 2021 21:37
@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@mockersf mockersf marked this pull request as ready for review February 4, 2022 19:21
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 22, 2022
@mockersf
Copy link
Member Author

mockersf commented Jun 1, 2022

closing as too controversial

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 X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants