-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Spawning scenes with cameras no longer works #11852
Comments
I think we should fix this for 0.13. |
Can/should we derive Reflect on our bundle types to more easily catch these errors? |
Yeah I think thats a good call. Probably not for 0.13 though. |
Should we just enforce labels to derive reflect and then we reflect them via their path? |
Alrighty after reverting #10644 and adding a reflect_value impl to use crate as bevy_reflect;
use bevy_reflect_derive::impl_reflect_value;
impl_reflect_value!(::wgpu::TextureUsages(Debug, Hash, PartialEq)); It is broken for serialization scenarios (as TextureUsages is not serializable). But it fixes GLTF loading. I'm going to see if we can do the same reflect_value hack for #10644 to avoid needing to revert (but note that us moving away from strings for this will break serialization for that as well). The TextureUsages serialization can be fixed by moving to the Image mirror types we'll be adding here: #11294 (comment)
The interned RenderGraph label serialization will be its own can of worms as we've moved to using types as identifiers. We'd likely need some type of TypePath -> label object registry for this. My new scene system would accommodate this type of exchange by supporting constructing the runtime types from different types in the scene, with access to a World context. (aka use the TypePath in the scene to grab the TypePath->Object mapping from a resource when spawning). |
Arg adding |
Sorry about #11412 not implementing reflect. I noticed the issue a while ago but completely forgot about fixing it or mentioning it. But yeah, should be easy enough to just impl Reflect for it. |
Maybe we should define a custom bevy enum that is easy to reflect and match on it when we need the wgpu type? |
Yeah thats where my head is at as well / what I was getting at with this comment:
|
…raMainTextureUsages (#11878) # Objective The new render graph labels do not (and cannot) implement normal Reflect, which breaks spawning scenes with cameras (including GLTF scenes). Likewise, the new `CameraMainTextureUsages` also does not (and cannot) implement normal Reflect because it uses `wgpu::TextureUsages` under the hood. Fixes #11852 ## Solution This implements minimal "reflect value" for `CameraRenderGraph` and `CameraMainTextureUsages` and registers the types, which satisfies our spawn logic. Note that this _does not_ fix scene serialization for these types, which will require more significant changes. We will especially need to think about how (and if) "interned labels" will fit into the scene system. For the purposes of 0.13, I think this is the best we can do. Given that this serialization issue is prevalent throughout Bevy atm, I'm ok with adding a couple more to the pile. When we roll out the new scene system, we will be forced to solve these on a case-by-case basis. --- ## Changelog - Implement Reflect (value) for `CameraMainTextureUsages` and `CameraRenderGraph`, and register those types.
Bevy version
77c26f6
What you did
Exported a GLTF scene from Blender with a camera and loaded it in Bevy.
What went wrong
The app crashed because CameraRenderGraph does not implement Reflect. This regressed in #10644.
For future reference, removing Reflect from a type that exists in a bundle used in scenes should almost never be allowed.
The text was updated successfully, but these errors were encountered: