-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Split AmbientLight
into two
#21595
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
base: main
Are you sure you want to change the base?
Split AmbientLight
into two
#21595
Conversation
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. |
use bevy_ecs::prelude::*; | ||
use bevy_reflect::prelude::*; | ||
|
||
/// An ambient light, which lights the entire scene equally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to crosslink to the override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few doc notes, and needs a migration guide. But I like the core idea: I think that this is less confusing.
Objective
For resources-as-components (#19731), structs mustn't doubly derive both
Component
andResource
.Solution
Split
AmbientLight
in two:AmbientLight
(the resource) andAmbientLightOverride
(the component).Testing
I initially made two structs
AmbientLightComponent
andAmbientLightResource
, and replaced every mention with the relevant one to ensure that I didn't confuse the two.Notes