Skip to content

Conversation

Trashtalk217
Copy link
Contributor

@Trashtalk217 Trashtalk217 commented Oct 18, 2025

Objective

For resources-as-components (#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.

@Trashtalk217 Trashtalk217 added D-Trivial Nice and easy! A great choice to get started with Bevy A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 18, 2025
@Trashtalk217 Trashtalk217 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Oct 18, 2025
@JMS55 JMS55 requested a review from SparkyPotato October 19, 2025 02:16
@mgi388
Copy link
Contributor

mgi388 commented Oct 19, 2025

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?

@Igor-dvr
Copy link

Related #18207

@Trashtalk217
Copy link
Contributor Author

@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 alice-i-cecile added D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed D-Trivial Nice and easy! A great choice to get started with Bevy labels Oct 19, 2025
use bevy_ecs::prelude::*;
use bevy_reflect::prelude::*;

/// An ambient light, which lights the entire scene equally.
Copy link
Member

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.
Copy link
Member

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.

Copy link
Member

@alice-i-cecile alice-i-cecile left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants