Optimize ShaderDefs updates

[yrns]

• change ShaderDefIterator to iterate every ShaderDef defined or not
• remove clear system, and only update on change

Related to #63.

[cart]

I expect this to behave incorrectly when an asset isn't loaded yet. Ex:

1. begin loading a "shader def" asset and assign its handle to a component
2. this system runs. the entity with the handle shows up in the query iteration because the handle was changed
3. it gets filtered out because the asset isn't loaded yet
4. later, when the asset finishes loading, this system doesn't run on the entity we skipped, because the Changed flag is no longer set.

[cart]

Good call! This is definitely something we should do incrementally. Just one hangup (see comment above)

[yrns]

Hmm, this doesn't happen in practice with StandardMaterial or ColorMaterial. You'd have to be deserializing ShaderDefs off disk or something? So I should track empty handles and wait for asset creation events later?

[cart]

Yeah currently its not a huge problem because StandardMaterial and ColorMaterial are generally created "immediately". But as soon as we start loading materials from disk, we'll hit this problem (ex: its probably already a problem when using StandardMaterial handles from a still-loading gltf file)

Your suggested fix is how we should do it. We also need to account for "asset change events". Because the value of the asset could change after loading it.

I think using the general approach we use for meshes is probably the right call:

bevy/crates/bevy_render/src/mesh/mesh.rs

Line 334 in 9239621

pub fn mesh_resource_provider_system(

Another Problem

remove clear system, and only update on change

I'm now realizing that this breaks down when the asset "changes". Doing this "incrementally" is actually pretty hard. We would need to:

1. remove the old defs associated with the asset (which we no longer have the asset value for).
2. If another asset provided the same def (unlikely but possible), we would need to ensure we don't remove that def because its still valid
3. insert the new defs

This is do-able, but not with the current design. Rather than merge them all into a single ShaderDefs, maybe it should be a key->value mapping that gets merged when we actually compile the shader. That way we could "replace" the shader defs for a specific context without knowing about the global context (or needing to know the past values for a given context).

[yrns]

I tried getting it to happen with the gltf example. The stock impls of ShaderDefs don't care if the sub-assets are loaded, only that the Option/bool is set. I can make a test case where it happens.

So the key is the asset handle? And log warnings if there are collisions when merged?

[cart]

Yeah the key should probably be HandleUntyped so that we can support multiple handle types.

[cart]

I think collisions shouldn't be a problem. Can you see that producing undesirable behavior?

[yrns]

@cart, this works as-is, minus some cleanup (mainly reverting the macro changes). I'm wondering what you think of not tracking entities <-> handles like mesh_resource_provider_system does. I started into it then realized entities were never removed from the hashmap, so I went with this simpler version to start. It's slower but I imagine asset updates are infrequent? Also, does this tracking of shader defs per-component via Uuid look right? Nothing currently uses shader_defs_system.

It seems like there's a case here for a nicer abstraction for systems that deal with changing assets/handles. I see this pattern appears in a few places, but I haven't looked at all of them.

[cart]

Sorry for the delay. Diving back in now!

[cart]

Can we use the TypeId here instead of TypeUuid? So far we aren't requiring TypeUuid on components and we use TypeId as the unique identifier everywhere else.

[cart]

(Assets do require TypeUuid though)

[cart]

I'm wondering what you think of not tracking entities <-> handles like mesh_resource_provider_system does. I started into it then realized entities were never removed from the hashmap, so I went with this simpler version to start. It's slower but I imagine asset updates are infrequent?

Yeah I think this approach works great. Good call!

Also, does this tracking of shader defs per-component via Uuid look right? Nothing currently uses shader_defs_system.

We should use TypeId here for now (see comment above)

It seems like there's a case here for a nicer abstraction for systems that deal with changing assets/handles. I see this pattern appears in a few places, but I haven't looked at all of them.

Agreed. If we can find a better pattern I'm definitely sold!

[cart]

In general this approach looks good to me. Lets solve the FIX items, rebase / resolve conflicts, and transition out of draft mode!

[DJMcNab]

This still seems like a useful change - it seems that we still clear all the shader defs each frame (!) on main

However, I that this will get integrated into the upcoming render rework; if you don't want to continue pushing this forward, feel free to close it @yrns.

I suspect rebasing this PR will not be useful at the moment, as the render rework is in flight - we can check back in once that lands to check the relevance of these changes then.