Move AmbientLight to a component, instead of a resource.

[Cluster7ck]

Objective

Fixes #7193

Solution

• Changed AmbientLight from a Resource to a Component.
• The PbrPlugin adds a default AmbientLight to each camera in PostUpdate. I suspect this doesn't make much sense and should be configurable somewhere else? Not sure. If this would live somewhere else, that would also get rid of SimulationLightSystems::AddAmbientLights which seems misplaced.
• The component is extracted in ReanderStage::Extract. Currently the data is put into a dedicated ExtractedAmbientLights struct. This should probably be a more general ExtractedCameraConfig struct or similar, thoughts?

---

Changelog

• Ambient lights can now be configured on a per camera basis.

Migration Guide

Previously setting up an AmbientLight looked something like this:

fn main() {
  App::new()
    ...
    .add_startup_system(setup)
    .insert_resource(AmbientLight {
        color: Color::WHITE,
        brightness: 1.0,
    })
    ...
    .run();
}

fn setup(mut commands: Commands) {
  commands.spawn((
        Camera3dBundle {
            ..default()
        },
        ...,
    ));
}

With AmbientLight as a per camera component the code needs to be transformed into:

fn main() {
  App::new()
      ...
      .add_startup_system(setup)
      ...
      .run();
}

fn setup(mut commands: Commands) {
  commands.spawn((
      Camera3dBundle {
          ..default()
      },
      AmbientLight {
          color: Color::WHITE,
          brightness: 1.0,
      },
      ...,
  ));
}

AmbientLight can be directly added to the camera entity as a component

[nicopap]

Concerning migration: You'll need to update some examples:

examples/tools/scene_viewer/main.rs
examples/stress_tests/many_foxes.rs
examples/animation/animated_transform.rs
examples/animation/gltf_skinned_mesh.rs
examples/animation/animated_fox.rs
examples/animation/custom_skinned_mesh.rs
examples/ecs/iter_combinations.rs
examples/3d/load_gltf.rs
examples/3d/spotlight.rs
examples/3d/skybox.rs
examples/3d/lighting.rs

It should be as simple as removing the .insert_resouce(AmbientLight…) and adding a ambient_light: AmbientLight {…}, field to the Camera3dBundle the example spawns.

[nicopap]

This would add the component every frame to every cameras, overwriting the previous value. This would prevent users from setting their own ambient light. You shouldn't need such system. Instead, it's likely the best solution is to add the AmbientLight component to Camera3dBundle.

[Cluster7ck]

Thanks for taking the time to give feedback!

Yeah I kind of arrived at the same conclusion but I am now facing a different issue. Currently AmbientLight is part of the pbr crate/feature and is only ever used there. Does this mean it should be behind a #[cfg(feature = "bevy_pbr")] directive?:

#[derive(Bundle)]
pub struct Camera3dBundle {
    pub camera: Camera,
    pub camera_render_graph: CameraRenderGraph,
    pub projection: Projection,
    pub visible_entities: VisibleEntities,
    pub frustum: Frustum,
    pub transform: Transform,
    pub global_transform: GlobalTransform,
    pub camera_3d: Camera3d,
    pub tonemapping: Tonemapping,
    #[cfg(feature = "bevy_pbr")]
    pub ambient_light: AmbientLight,
}

Or do we just move the whole struct into the bevy_core_pipeline crate? Are there other options I'm overlooking?

[nicopap]

yeah, this is a tough decision, because ambient light relates to pbr while core_pipeline is just the generic bevy rendering infra (no expert actually, might be wrong).

An alternative would be to not add AmbientLight to Camera3dBundle and assume default when the component is not present on the camera entity. (that is, in the prepare_lights system, you'd accept a Option<&ExtractedAmbientLight> instead of &ExtractedAmbientLight, and go with the default value when None) It's easy for the user to do:

commands.spawn((
  Camera3dBundle {
    //...
  }),
  AmbientLight { … },
))

If they want a custom ambient light.

The cfg feature would require bevy_core_pipeline to depend on bevy_pbr which is not OK IMO because of circular dependency (even on feature gate). I would really prefer if this was a field of Camera3dBundle, but it seems not possible. Moving the struct to bevy_core_pipeline seems fine, but it would be misleading (ie: it does nothing when not using bevy_pbr). Moving the struct to bevy_core_pipeline and toggling it on feature flag seems overly complex.

[Cluster7ck]

An alternative would be to not add AmbientLight to Camera3dBundle and assume default when the component is not present on the camera entity. (that is, in the prepare_lights system, you'd accept a Option<&ExtractedAmbientLight> instead of &ExtractedAmbientLight, and go with the default value when None)

This is the approach I'm currently taking, but it's definitely harder to discover as a user. I will finish the implementation with this direction and maybe then a SME (have to look up who) can chime in. This seems relevant because #7194 will probably deal with a similar issue. So will any another (future) per-camera feature.

[nicopap]

I agree, it makes it not discoverable at all, that's why I'd prefer it to be a field of Camera3dBundle. Looks like someone already did pick up #7194 with: #7215 bevy_core_pipeline depends on bevy_render, which is what defines Msaa, so it doesn't have the same issue.

[mockersf]

If we want to go forward with this, I think it should behave like UiCameraConfig. Default behaviour if not present, custom if present

[Cluster7ck]

Does bors rebase and squash this or do I need to do that?

[nicopap]

bors rebases and squashes. yeah

[mockersf]

adding Controversial label, neither the original issue or this PR has a good justification on why to split ambient light per camera

[Cluster7ck]

adding Controversial label, neither the original issue or this PR has a good justification on why to split ambient light per camera

For the case where you only have one camera and a custom AmbientLight this is a matter of: Is the migration worth it?
Where someone used multiple cameras and a custom AmbientLight resource before, they now have to add multiple (presumably equivalent) components to all their cameras. The question here is: Is that overhead worth the flexibility?

A feature where this might be interesting:
You want to display 3D objects in your 2D UI. Think inventory, shop or character selection. If you overlay this UI over your scene, you probably want to render the 3D-UI into its own buffer with different lighting and then composite with the scene and 2D UI.

[mockersf]

Even if we want multiple ambient light, I don't think it should be a component of the camera. Ambient light should then be its own entity, and respect render layers.

[bas-ie]

Backlog cleanup: closing in favour of tracking issue #7193 due to inactivity, lack of consensus.