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

ParticleTextureModifier serialization #192

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

yrns
Copy link
Contributor

@yrns yrns commented Jun 27, 2023

This is a way to handle serialization of particle textures within EffectAsset by way of:

  • Storing the texture asset path with the handle in an AssetHandle, which derefs to Handle and de/serializes as an AssetPath.
  • Loading particle textures in the EffectAssetLoader as dependent assets to the effect.

I included a new example in which you can save the current effect to the assets directory, edit the file, and click "load" to respawn the effect with the new asset.

I found the idea here. I believe AssetHandle becomes unnecessary with the new asset rework (bevyengine/bevy#8624) in Bevy 0.12.

@djeedai
Copy link
Owner

djeedai commented Jun 28, 2023

Thanks for the PR! That looks definitely very useful, and is something I'd like for all modifiers so we can finally properly serialize EffectAsset and users can start building collections of effects without having to maintain a lot of code.

I was wondering how we can do that in a future-proof way. It looks like a good solution, however I'm wondering if that's the right solution. Surely we're not the first ones using Bevy to have this issue. So it feels awkward to have to redefine our own AssetHandle<T>. Is there really no built-in solution in Bevy for that? Should this be fixed in Bevy first, instead of rolling a custom solution inside this library (which won't be compatible with any future Bevy built-in support)? And if that becomes unnecessary with the AssetV2 pipeline, should we just wait for that? Otherwise how do people using that new system transition later to AssetV2? I'd hate that people start serializing many assets then find themselves stuck with an unsupported serialization format, and have to redo all their assets because we moved to AssetV2 and they're not compatible.

In short, I need to think a bit more about this.

@djeedai djeedai added C - enhancement New feature or request C - breaking change A breaking API or behavior change A - components Change related to an ECS component labels Jun 28, 2023
@yrns
Copy link
Contributor Author

yrns commented Jun 28, 2023

It's unfortunate Bevy doesn't handle this situation more generally. This doesn't even work with scenes, which is where I would expect most users of Bevy to have handles to external assets. I have seen a few complaints, but I guess most people aren't writing their own asset loaders.

Another possible solution is scanning all the assets at startup, or keeping a persisted list of assets with ids and paths to reference at load. Since the asset ids are stable hashes, the path can be looked up from there. I suspect both of those options would be more code, and more steps overall than what's in this PR. Handle is Reflect but doesn't serialize out of the box so it'll be a special case no matter what. Unless you serialize assets using reflection which is another whole can of worms.

In regards to future Bevy changes, you have your own asset loader so the format shouldn't change unless EffectAsset changes. The question is whether you want to support serialization at all. Should users of this library expect effects to be future-compatible with later versions of Hanabi and Bevy? I would think not, but I'm not sure. The thing is, you already support serialization, it just doesn't work with ParticleTextureModifier.

You also might consider putting Serialize/Deserialize behind a non-default feature, making it opt-in.

@djeedai
Copy link
Owner

djeedai commented Jul 1, 2023

Ok from quick discussion on Discord with Cart it doesn't seem like there's a better alternative at the minute, so I guess it's fine merging the current solution. I merged the Graph API though (#191) so there's a few conflicts now, if you have time to resolve @yrns I can review after. Thanks!

@yrns
Copy link
Contributor Author

yrns commented Jul 2, 2023

@djeedai Merged, and added a test.

@djeedai
Copy link
Owner

djeedai commented Jul 11, 2023

I'm so sorry, I merged a bunch of things I had locally and I made more conflicts 🤦‍♂️

@djeedai
Copy link
Owner

djeedai commented Jul 11, 2023

Ok I fixed the issues on your branch directly. However I'd like to test the new example, but I can't do that until bevy-inspector-egui ugpraded. So leaving in standby for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A - components Change related to an ECS component C - breaking change A breaking API or behavior change C - enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants