Skip to content

ParticleTextureModifier serialization#192

Open
yrns wants to merge 6 commits into
djeedai:mainfrom
yrns:ser-tex
Open

ParticleTextureModifier serialization#192
yrns wants to merge 6 commits into
djeedai:mainfrom
yrns:ser-tex

Conversation

@yrns

@yrns yrns commented Jun 27, 2023

Copy link
Copy Markdown
Contributor

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

djeedai commented Jun 28, 2023

Copy link
Copy Markdown
Owner

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

yrns commented Jun 28, 2023

Copy link
Copy Markdown
Contributor Author

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

djeedai commented Jul 1, 2023

Copy link
Copy Markdown
Owner

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

yrns commented Jul 2, 2023

Copy link
Copy Markdown
Contributor Author

@djeedai Merged, and added a test.

@djeedai

djeedai commented Jul 11, 2023

Copy link
Copy Markdown
Owner

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

@djeedai

djeedai commented Jul 11, 2023

Copy link
Copy Markdown
Owner

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.

@djeedai djeedai added the D - needs design Need to clarify the design before work can move forward label Mar 16, 2025
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 D - needs design Need to clarify the design before work can move forward

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants