Address review comments

crates/bevy_pbr/src/light_probe/environment_map.rs

@@ -9,7 +9,11 @@
 //! entities they're attached to have:
 //!
 //! 1. If attached to a view, they represent the objects located a very far
-//!    distance from the view, in a similar manner to a skybox.
+//!    distance from the view, in a similar manner to a skybox. Essentially, these
+//!    *view environment maps* represent a higher-quality replacement for
+//!    [`AmbientLight`] for outdoor scenes. The indirect light from such
+//!    environment maps are added to every point of the scene, including interior
+//!    enclosed areas.
 //!
 //! 2. If attached to a [`LightProbe`], environment maps represent the immediate
 //!    surroundings of a specific location in the scene. These types of
@@ -18,16 +22,29 @@
 //!    these to a scene.
 //!
 //! Typically, environment maps are static (i.e. "baked", calculated ahead of
-//! time) and so only reflect fixed static geometry. Environment map textures
-//! can be generated from panoramas via the [glTF IBL Sampler].
+//! time) and so only reflect fixed static geometry. The environment maps must
+//! be pre-filtered into a pair of cubemaps, one for the diffuse component and
+//! one for the specular component, according to the [split-sum approximation].
+//! To pre-filter your environment map, you can use the [glTF IBL Sampler] or
+//! its [artist-friendly UI]. The diffuse map uses the Lambertian distribution,
+//! while the specular map uses the GGX distribution.
+//!
+//! The Khronos Group has [several pre-filtered environment maps] available for
+//! you to use.
 //!
 //! Currently, reflection probes (i.e. environment maps attached to light
 //! probes) use binding arrays (also known as bindless textures) and
-//! consequently aren't supported on WebGL 2 or WebGPU. Reflection probes are
+//! consequently aren't supported on WebGL2 or WebGPU. Reflection probes are
 //! also unsupported if GLSL is in use, due to `naga` limitations. Environment
 //! maps attached to views are, however, supported on all platforms.
 //!
+//! [split-sum approximation]: https://cdn2.unrealengine.com/Resources/files/2013SiggraphPresentationsNotes-26915738.pdf
+//!
 //! [glTF IBL Sampler]: https://github.com/KhronosGroup/glTF-IBL-Sampler
+//!
+//! [artist-friendly UI]: https://github.com/pcwalton/gltf-ibl-sampler-egui
+//!
+//! [several pre-filtered environment maps]: https://github.com/KhronosGroup/glTF-Sample-Environments
 
 use bevy_asset::{AssetId, Handle};
 use bevy_ecs::{
@@ -44,6 +61,7 @@ use bevy_render::{
     },
     texture::{FallbackImage, Image},
 };
+use std::ops::Deref;
 
 #[cfg(all(not(feature = "shader_format_glsl"), not(target_arch = "wasm32")))]
 use bevy_utils::HashMap;
@@ -81,8 +99,8 @@ pub(crate) struct EnvironmentMapIds {
     pub(crate) specular: AssetId<Image>,
 }
 
-/// A convenient bundle that contains everything needed to make an entity a
-/// reflection probe.
+/// A bundle that contains everything needed to make an entity a reflection
+/// probe.
 ///
 /// A reflection probe is a type of environment map that specifies the light
 /// surrounding a region in space. For more information, see
@@ -140,10 +158,10 @@ pub(crate) struct RenderViewBindGroupEntries<'a> {
     ///
     /// This is a vector of `wgpu::TextureView`s. But we don't want to import
     /// `wgpu` in this crate, so we refer to it indirectly like this.
-    diffuse_texture_views: Vec<&'a <TextureView as std::ops::Deref>::Target>,
+    diffuse_texture_views: Vec<&'a <TextureView as Deref>::Target>,
 
     /// As above, but for specular cubemaps.
-    specular_texture_views: Vec<&'a <TextureView as std::ops::Deref>::Target>,
+    specular_texture_views: Vec<&'a <TextureView as Deref>::Target>,
 
     /// The sampler used to sample elements of both `diffuse_texture_views` and
     /// `specular_texture_views`.
@@ -187,8 +205,7 @@ impl RenderViewEnvironmentMaps {
 
 #[cfg(all(not(feature = "shader_format_glsl"), not(target_arch = "wasm32")))]
 impl RenderViewEnvironmentMaps {
-    /// Returns true if there are no environment maps for this view or false if
-    /// there are such environment maps.
+    /// Whether there are no environment maps associated with the view.
     pub(crate) fn is_empty(&self) -> bool {
         self.binding_index_to_cubemap.is_empty()
     }
@@ -336,7 +353,7 @@ impl<'a> RenderViewBindGroupEntries<'a> {
 /// populates `sampler` if this is the first such view.
 #[cfg(all(not(feature = "shader_format_glsl"), not(target_arch = "wasm32")))]
 fn add_texture_view<'a>(
-    texture_views: &mut Vec<&'a <TextureView as std::ops::Deref>::Target>,
+    texture_views: &mut Vec<&'a <TextureView as Deref>::Target>,
     sampler: &mut Option<&'a Sampler>,
     image_id: AssetId<Image>,
     images: &'a RenderAssets<Image>,
@@ -362,17 +379,13 @@ fn add_texture_view<'a>(
 impl<'a> RenderViewBindGroupEntries<'a> {
     /// Returns a list of texture views of each diffuse cubemap, in binding
     /// order.
-    pub(crate) fn diffuse_texture_views(
-        &'a self,
-    ) -> &'a [&'a <TextureView as std::ops::Deref>::Target] {
+    pub(crate) fn diffuse_texture_views(&'a self) -> &'a [&'a <TextureView as Deref>::Target] {
         self.diffuse_texture_views.as_slice()
     }
 
     /// Returns a list of texture views of each specular cubemap, in binding
     /// order.
-    pub(crate) fn specular_texture_views(
-        &'a self,
-    ) -> &'a [&'a <TextureView as std::ops::Deref>::Target] {
+    pub(crate) fn specular_texture_views(&'a self) -> &'a [&'a <TextureView as Deref>::Target] {
         self.specular_texture_views.as_slice()
     }
 }

crates/bevy_pbr/src/light_probe/environment_map.wgsl

@@ -21,12 +21,7 @@ fn environment_map_light(
 ) -> EnvironmentMapLight {
     var out: EnvironmentMapLight;
 
-    // Split-sum approximation for image based lighting: https://cdn2.unrealengine.com/Resources/files/2013SiggraphPresentationsNotes-26915738.pdf
-    // Technically we could use textureNumLevels(environment_map_specular) - 1 here, but we use a uniform
-    // because textureNumLevels() does not work on WebGL2
-    let radiance_level = perceptual_roughness * f32(bindings::lights.environment_map_smallest_specular_mip_level);
-
-#ifdef ENVIRONMENT_MAP_LIGHT_PROBES
+#ifdef MULTIPLE_LIGHT_PROBES_IN_ARRAY
     // Search for a reflection probe that contains the fragment.
     //
     // TODO: Interpolate between multiple reflection probes.
@@ -36,8 +31,7 @@ fn environment_map_light(
             reflection_probe_index += 1) {
         let reflection_probe = light_probes.reflection_probes[reflection_probe_index];
 
-        // Transpose the inverse transpose transform to recover the inverse
-        // transform.
+        // Unpack the inverse transform.
         let inverse_transpose_transform = mat4x4<f32>(
             reflection_probe.inverse_transpose_transform[0],
             reflection_probe.inverse_transpose_transform[1],
@@ -56,7 +50,7 @@ fn environment_map_light(
 
     // If we didn't find a reflection probe, use the view environment map if applicable.
     if (cubemap_index < 0) {
-        cubemap_index = light_probes.cubemap_index;
+        cubemap_index = light_probes.view_cubemap_index;
     }
 
     // If there's no cubemap, bail out.
@@ -66,6 +60,9 @@ fn environment_map_light(
         return out;
     }
 
+    // Split-sum approximation for image based lighting: https://cdn2.unrealengine.com/Resources/files/2013SiggraphPresentationsNotes-26915738.pdf
+    let radiance_level = perceptual_roughness * f32(textureNumLevels(bindings::specular_environment_maps[cubemap_index]) - 1u);
+
     let irradiance = textureSampleLevel(
         bindings::diffuse_environment_maps[cubemap_index],
         bindings::environment_map_sampler,
@@ -77,13 +74,18 @@ fn environment_map_light(
         bindings::environment_map_sampler,
         vec3(R.xy, -R.z),
         radiance_level).rgb;
-#else   // ENVIRONMENT_MAP_LIGHT_PROBES
-    if (light_probes.cubemap_index < 0) {
+#else   // MULTIPLE_LIGHT_PROBES_IN_ARRAY
+    if (light_probes.view_cubemap_index < 0) {
         out.diffuse = vec3(0.0);
         out.specular = vec3(0.0);
         return out;
     }
 
+    // Split-sum approximation for image based lighting: https://cdn2.unrealengine.com/Resources/files/2013SiggraphPresentationsNotes-26915738.pdf
+    // Technically we could use textureNumLevels(specular_environment_map) - 1 here, but we use a uniform
+    // because textureNumLevels() does not work on WebGL2
+    let radiance_level = perceptual_roughness * f32(light_probes.smallest_specular_mip_level_for_view);
+
     let irradiance = textureSampleLevel(
         bindings::diffuse_environment_map,
         bindings::environment_map_sampler,
@@ -95,7 +97,7 @@ fn environment_map_light(
         bindings::environment_map_sampler,
         vec3(R.xy, -R.z),
         radiance_level).rgb;
-#endif  // ENVIRONMENT_MAP_LIGHT_PROBES
+#endif  // MULTIPLE_LIGHT_PROBES_IN_ARRAY
 
     // No real world material has specular values under 0.02, so we use this range as a
     // "pre-baked specular occlusion" that extinguishes the fresnel term, for artistic control.

crates/bevy_pbr/src/light_probe/mod.rs

@@ -46,7 +46,8 @@ pub const MAX_VIEW_REFLECTION_PROBES: usize = 8;
 /// cubemaps applied to all objects that a view renders.
 pub struct LightProbePlugin;
 
-/// A cuboid region that provides global illumination to all fragments inside it.
+/// A marker component for a light probe, which is a cuboid region that provides
+/// global illumination to all fragments inside it.
 ///
 /// The light probe range is conceptually a unit cube (1×1×1) centered on the
 /// origin.  The [`bevy_transform::prelude::Transform`] applied to this entity
@@ -86,7 +87,11 @@ pub struct LightProbesUniform {
     /// The index of the diffuse and specular environment maps associated with
     /// the view itself. This is used as a fallback if no reflection probe in
     /// the list contains the fragment.
-    cubemap_index: i32,
+    view_cubemap_index: i32,
+
+    /// The smallest valid mipmap level for the specular environment cubemap
+    /// associated with the view.
+    smallest_specular_mip_level_for_view: u32,
 }
 
 /// A map from each camera to the light probe uniform associated with it.
@@ -132,7 +137,8 @@ impl Plugin for LightProbePlugin {
             Shader::from_wgsl
         );
 
-        app.register_type::<LightProbe>();
+        app.register_type::<LightProbe>()
+            .register_type::<EnvironmentMapLight>();
     }
 
     fn finish(&self, app: &mut App) {
@@ -247,7 +253,8 @@ impl Default for LightProbesUniform {
         Self {
             reflection_probes: [RenderReflectionProbe::default(); MAX_VIEW_REFLECTION_PROBES],
             reflection_probe_count: 0,
-            cubemap_index: -1,
+            view_cubemap_index: -1,
+            smallest_specular_mip_level_for_view: 0,
         }
     }
 }
@@ -267,25 +274,34 @@ impl LightProbesUniform {
     ) -> (LightProbesUniform, RenderViewEnvironmentMaps) {
         let mut render_view_environment_maps = RenderViewEnvironmentMaps::new();
 
+        // Find the index of the cubemap associated with the view, and determine
+        // its smallest mip level.
+        let (mut view_cubemap_index, mut smallest_specular_mip_level_for_view) = (-1, 0);
+        if let Some(EnvironmentMapLight {
+            diffuse_map: diffuse_map_handle,
+            specular_map: specular_map_handle,
+        }) = view_environment_maps
+        {
+            if let (Some(_), Some(specular_map)) = (
+                image_assets.get(diffuse_map_handle),
+                image_assets.get(specular_map_handle),
+            ) {
+                view_cubemap_index =
+                    render_view_environment_maps.get_or_insert_cubemap(&EnvironmentMapIds {
+                        diffuse: diffuse_map_handle.id(),
+                        specular: specular_map_handle.id(),
+                    }) as i32;
+                smallest_specular_mip_level_for_view = specular_map.mip_level_count - 1;
+            }
+        };
+
         // Initialize the uniform to only contain the view environment map, if
         // applicable.
         let mut uniform = LightProbesUniform {
             reflection_probes: [RenderReflectionProbe::default(); MAX_VIEW_REFLECTION_PROBES],
             reflection_probe_count: light_probes.len().min(MAX_VIEW_REFLECTION_PROBES) as i32,
-            cubemap_index: match view_environment_maps {
-                Some(EnvironmentMapLight {
-                    diffuse_map,
-                    specular_map,
-                }) if image_assets.get(diffuse_map).is_some()
-                    && image_assets.get(specular_map).is_some() =>
-                {
-                    render_view_environment_maps.get_or_insert_cubemap(&EnvironmentMapIds {
-                        diffuse: diffuse_map.id(),
-                        specular: specular_map.id(),
-                    }) as i32
-                }
-                _ => -1,
-            },
+            view_cubemap_index,
+            smallest_specular_mip_level_for_view,
         };
 
         // Add reflection probes from the scene, if supported by the current

crates/bevy_pbr/src/render/light.rs

@@ -196,7 +196,6 @@ pub struct GpuLights {
     n_directional_lights: u32,
     // offset from spot light's light index to spot light's shadow map index
     spot_light_shadowmap_offset: i32,
-    environment_map_smallest_specular_mip_level: u32,
 }
 
 // NOTE: this must be kept in sync with the same constants in pbr.frag
@@ -962,7 +961,6 @@ pub fn prepare_lights(
             // index to shadow map index, we need to subtract point light count and add directional shadowmap count.
             spot_light_shadowmap_offset: num_directional_cascades_enabled as i32
                 - point_light_count as i32,
-            environment_map_smallest_specular_mip_level: 0, /* FIXME */
         };
 
         // TODO: this should select lights based on relevance to the view instead of the first ones that show up in a query

crates/bevy_pbr/src/render/mesh.rs

@@ -845,7 +845,7 @@ impl SpecializedMeshPipeline for MeshPipeline {
         ));
 
         #[cfg(all(not(feature = "shader_format_glsl"), not(target_arch = "wasm32")))]
-        shader_defs.push("ENVIRONMENT_MAP_LIGHT_PROBES".into());
+        shader_defs.push("MULTIPLE_LIGHT_PROBES_IN_ARRAY".into());
 
         let format = if key.contains(MeshPipelineKey::HDR) {
             ViewTarget::TEXTURE_FORMAT_HDR

crates/bevy_pbr/src/render/mesh_view_bindings.wgsl

@@ -37,7 +37,7 @@
 
 @group(0) @binding(12) var screen_space_ambient_occlusion_texture: texture_2d<f32>;
 
-#ifdef ENVIRONMENT_MAP_LIGHT_PROBES
+#ifdef MULTIPLE_LIGHT_PROBES_IN_ARRAY
 @group(0) @binding(13) var diffuse_environment_maps: binding_array<texture_cube<f32>>;
 @group(0) @binding(14) var specular_environment_maps: binding_array<texture_cube<f32>>;
 #else

crates/bevy_pbr/src/render/mesh_view_types.wgsl

@@ -121,6 +121,10 @@ struct LightProbes {
     // This must match `MAX_VIEW_REFLECTION_PROBES` on the Rust side.
     reflection_probes: array<ReflectionProbe, 8u>,
     reflection_probe_count: i32,
-    // -1 if no cubemap is associated.
-    cubemap_index: i32,
+    // The index of the view environment map cubemap binding, or -1 if there's
+    // no such cubemap.
+    view_cubemap_index: i32,
+    // The smallest valid mipmap level for the specular environment cubemap
+    // associated with the view.
+    smallest_specular_mip_level_for_view: u32,
 };

examples/3d/reflection_probes.rs

@@ -224,7 +224,7 @@ fn change_reflection_type(
 
 // A system that handles enabling and disabling rotation.
 fn toggle_rotation(keyboard: Res<ButtonInput<KeyCode>>, mut app_status: ResMut<AppStatus>) {
-    if keyboard.just_pressed(KeyCode::Return) {
+    if keyboard.just_pressed(KeyCode::Enter) {
         app_status.rotating = !app_status.rotating;
     }
 }