Split AmbientLight into two
#21595
Split AmbientLight into two
#21595
Conversation
Trashtalk217
added
D-Trivial
Trashtalk217
added
the
M-Migration-Guide
|
Aren’t most games going to have many different 3D scenes and so the default ambient light is actually the special case here? I wonder if the special casing is backwards and we actually want DefaultAmbientLight and AmbientLight. Put another way, in my game I’m never going to use the resource one, I’m going to put an AmbientLight component on my different 3D cameras so it feels weird that with this PR I’d have to put an AmbientLightOverride component on those cameras—it’s not overriding anything either. It also has nice parity with SpotLight and PointLight. So would DefaultAmbientLight (resource) and AmbientLight (component) be a good option? |
|
Related #18207 |
|
@mgi388 It could very well be a good option. I'm completely ambivalent about the names, as long as there's a consensus among the rendering people. |
alice-i-cecile
added
D-Straightforward
| use bevy_ecs::prelude::*; | ||
| use bevy_reflect::prelude::*; | ||
|
|
||
| /// An ambient light, which lights the entire scene equally. |
There was a problem hiding this comment.
These docs need updating.
| /// An ambient light, which lights the entire scene equally. | ||
| /// | ||
| /// It can also be added to a camera to override the resource (or default) ambient for that camera only. | ||
| /// This resource is inserted by the [`LightPlugin`] and by default it is set to a low ambient light. |
There was a problem hiding this comment.
This needs to crosslink to the override.
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
A few doc notes, and needs a migration guide. But I like the core idea: I think that this is less confusing.
atlv24
left a comment
atlv24
left a comment
There was a problem hiding this comment.
I would consider impl Into back and forth between them and using that instead of reconstructing in light.rs:1153 but thats a nit
LGTM i say merge
Co-authored-by: atlv <[email protected]>
alice-i-cecile
added
S-Ready-For-Final-Review
# Objective For resources-as-components (bevyengine#19731), structs mustn't doubly derive both `Component` and `Resource`. ## Solution Split `AmbientLight` in two: `AmbientLight` (the resource) and `AmbientLightOverride` (the component). ## Testing I initially made two structs `AmbientLightComponent` and `AmbientLightResource`, and replaced every mention with the relevant one to ensure that I didn't confuse the two. ## Notes - I don't know if the names are correct. I kept the easiest name for the resource, as that's the one most often used. - I haven't provided any conversion methods as there are already plans to replace the component variant with something else. --------- Co-authored-by: atlv <[email protected]>
5 participants
Objective
For resources-as-components (#19731), structs mustn't doubly derive both
ComponentandResource.Solution
Split
AmbientLightin two:AmbientLight(the resource) andAmbientLightOverride(the component).Testing
I initially made two structs
AmbientLightComponentandAmbientLightResource, and replaced every mention with the relevant one to ensure that I didn't confuse the two.Notes