From 5e3ae770ac89192e73f2473468b734d414ea0e16 Mon Sep 17 00:00:00 2001 From: ira Date: Mon, 29 May 2023 09:22:13 +0200 Subject: [PATCH 01/15] Fix screenshots on Wayland + Nvidia (#8701) # Objective Fix #8604 ## Solution Use `.add_srgb_suffix()` when creating the screenshot texture. Allow converting `Bgra8Unorm` images. Only a two line change for the fix, the `screenshot.rs` changes are just a bit of cleanup. --- .../bevy_render/src/texture/image_texture_conversion.rs | 2 +- crates/bevy_render/src/view/window.rs | 2 +- crates/bevy_render/src/view/window/screenshot.rs | 9 ++------- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/crates/bevy_render/src/texture/image_texture_conversion.rs b/crates/bevy_render/src/texture/image_texture_conversion.rs index 387d08b307..8a6f51852b 100644 --- a/crates/bevy_render/src/texture/image_texture_conversion.rs +++ b/crates/bevy_render/src/texture/image_texture_conversion.rs @@ -199,7 +199,7 @@ impl Image { .map(DynamicImage::ImageRgba8), // This format is commonly used as the format for the swapchain texture // This conversion is added here to support screenshots - TextureFormat::Bgra8UnormSrgb => ImageBuffer::from_raw( + TextureFormat::Bgra8UnormSrgb | TextureFormat::Bgra8Unorm => ImageBuffer::from_raw( self.texture_descriptor.size.width, self.texture_descriptor.size.height, { diff --git a/crates/bevy_render/src/view/window.rs b/crates/bevy_render/src/view/window.rs index 9bf79df741..d022d4cc53 100644 --- a/crates/bevy_render/src/view/window.rs +++ b/crates/bevy_render/src/view/window.rs @@ -377,7 +377,7 @@ pub fn prepare_windows( mip_level_count: 1, sample_count: 1, dimension: wgpu::TextureDimension::D2, - format: surface_configuration.format, + format: surface_configuration.format.add_srgb_suffix(), usage: TextureUsages::RENDER_ATTACHMENT | TextureUsages::COPY_SRC | TextureUsages::TEXTURE_BINDING, diff --git a/crates/bevy_render/src/view/window/screenshot.rs b/crates/bevy_render/src/view/window/screenshot.rs index ab7d9e8818..440634c274 100644 --- a/crates/bevy_render/src/view/window/screenshot.rs +++ b/crates/bevy_render/src/view/window/screenshot.rs @@ -234,16 +234,11 @@ impl SpecializedRenderPipeline for ScreenshotToScreenPipeline { shader: SCREENSHOT_SHADER_HANDLE.typed(), }, primitive: wgpu::PrimitiveState { - topology: wgpu::PrimitiveTopology::TriangleList, - strip_index_format: None, - front_face: wgpu::FrontFace::Ccw, cull_mode: Some(wgpu::Face::Back), - polygon_mode: wgpu::PolygonMode::Fill, - conservative: false, - unclipped_depth: false, + ..Default::default() }, depth_stencil: None, - multisample: wgpu::MultisampleState::default(), + multisample: Default::default(), fragment: Some(FragmentState { shader: SCREENSHOT_SHADER_HANDLE.typed(), entry_point: Cow::Borrowed("fs_main"), From 1ff4b987554590150d35626219fd8ec573a131f7 Mon Sep 17 00:00:00 2001 From: Jakob Hellermann Date: Mon, 29 May 2023 09:23:50 +0200 Subject: [PATCH 02/15] fix new clippy lints before they reach stable (#8700) # Objective - fix clippy lints early to make sure CI doesn't break when they get promoted to stable - have a noise-free `clippy` experience for nightly users ## Solution - `cargo clippy --fix` - replace `filter_map(|x| x.ok())` with `map_while(|x| x.ok())` to fix potential infinite loop in case of IO error --- examples/2d/mesh2d_manual.rs | 2 +- examples/3d/blend_modes.rs | 2 +- examples/3d/parallax_mapping.rs | 2 +- examples/3d/skybox.rs | 2 +- examples/3d/spotlight.rs | 2 +- examples/3d/tonemapping.rs | 2 +- examples/diagnostics/log_diagnostics.rs | 2 +- examples/games/contributors.rs | 2 +- examples/stress_tests/bevymark.rs | 2 +- examples/stress_tests/many_animated_sprites.rs | 2 +- examples/stress_tests/many_buttons.rs | 2 +- examples/stress_tests/many_cubes.rs | 2 +- examples/stress_tests/many_gizmos.rs | 2 +- examples/stress_tests/many_glyphs.rs | 2 +- examples/stress_tests/many_lights.rs | 2 +- examples/stress_tests/many_sprites.rs | 2 +- examples/stress_tests/text_pipeline.rs | 2 +- examples/stress_tests/transform_hierarchy.rs | 2 +- examples/ui/text.rs | 2 +- examples/window/window_resizing.rs | 2 +- 20 files changed, 20 insertions(+), 20 deletions(-) diff --git a/examples/2d/mesh2d_manual.rs b/examples/2d/mesh2d_manual.rs index 0ad142a561..0c55580d49 100644 --- a/examples/2d/mesh2d_manual.rs +++ b/examples/2d/mesh2d_manual.rs @@ -99,7 +99,7 @@ fn star( // We can now spawn the entities for the star and the camera commands.spawn(( // We use a marker component to identify the custom colored meshes - ColoredMesh2d::default(), + ColoredMesh2d, // The `Handle` needs to be wrapped in a `Mesh2dHandle` to use 2d rendering instead of 3d Mesh2dHandle(meshes.add(star)), // This bundle's components are needed for something to be rendered diff --git a/examples/3d/blend_modes.rs b/examples/3d/blend_modes.rs index 262728dbfc..4dd76d8faf 100644 --- a/examples/3d/blend_modes.rs +++ b/examples/3d/blend_modes.rs @@ -302,7 +302,7 @@ fn example_control_system( let randomize_colors = input.just_pressed(KeyCode::C); for (material_handle, controls) in &controllable { - let mut material = materials.get_mut(material_handle).unwrap(); + let material = materials.get_mut(material_handle).unwrap(); material.base_color.set_a(state.alpha); if controls.color && randomize_colors { diff --git a/examples/3d/parallax_mapping.rs b/examples/3d/parallax_mapping.rs index 31df37e37d..8260f9b3f3 100644 --- a/examples/3d/parallax_mapping.rs +++ b/examples/3d/parallax_mapping.rs @@ -378,7 +378,7 @@ fn update_normal( return; } if let Some(normal) = normal.0.as_ref() { - if let Some(mut image) = images.get_mut(normal) { + if let Some(image) = images.get_mut(normal) { image.texture_descriptor.format = TextureFormat::Rgba8Unorm; *already_ran = true; } diff --git a/examples/3d/skybox.rs b/examples/3d/skybox.rs index 0ce8237cd6..986114bfdb 100644 --- a/examples/3d/skybox.rs +++ b/examples/3d/skybox.rs @@ -145,7 +145,7 @@ fn asset_loaded( && asset_server.get_load_state(cubemap.image_handle.clone_weak()) == LoadState::Loaded { info!("Swapping to {}...", CUBEMAPS[cubemap.index].0); - let mut image = images.get_mut(&cubemap.image_handle).unwrap(); + let image = images.get_mut(&cubemap.image_handle).unwrap(); // NOTE: PNGs do not have any metadata that could indicate they contain a cubemap texture, // so they appear as one texture. The following code reconfigures the texture as necessary. if image.texture_descriptor.array_layer_count() == 1 { diff --git a/examples/3d/spotlight.rs b/examples/3d/spotlight.rs index d49ceb869a..38ee6e11b5 100644 --- a/examples/3d/spotlight.rs +++ b/examples/3d/spotlight.rs @@ -10,7 +10,7 @@ use rand::{thread_rng, Rng}; fn main() { App::new() .add_plugins(DefaultPlugins) - .add_plugin(FrameTimeDiagnosticsPlugin::default()) + .add_plugin(FrameTimeDiagnosticsPlugin) .add_plugin(LogDiagnosticsPlugin::default()) .add_systems(Startup, setup) .add_systems(Update, (light_sway, movement)) diff --git a/examples/3d/tonemapping.rs b/examples/3d/tonemapping.rs index 1d120081d8..6b9bb09eb0 100644 --- a/examples/3d/tonemapping.rs +++ b/examples/3d/tonemapping.rs @@ -430,7 +430,7 @@ fn update_color_grading_settings( mut selected_parameter: ResMut, ) { let method = tonemapping.single(); - let mut color_grading = per_method_settings.settings.get_mut(method).unwrap(); + let color_grading = per_method_settings.settings.get_mut(method).unwrap(); let mut dt = time.delta_seconds() * 0.25; if keys.pressed(KeyCode::Left) { dt = -dt; diff --git a/examples/diagnostics/log_diagnostics.rs b/examples/diagnostics/log_diagnostics.rs index 436bba684f..2cfb1e6cc8 100644 --- a/examples/diagnostics/log_diagnostics.rs +++ b/examples/diagnostics/log_diagnostics.rs @@ -9,7 +9,7 @@ fn main() { App::new() .add_plugins(DefaultPlugins) // Adds frame time diagnostics - .add_plugin(FrameTimeDiagnosticsPlugin::default()) + .add_plugin(FrameTimeDiagnosticsPlugin) // Adds a system that prints diagnostics to the console .add_plugin(LogDiagnosticsPlugin::default()) // Any plugin can register diagnostics diff --git a/examples/games/contributors.rs b/examples/games/contributors.rs index f5eb0462fe..1ca223c35d 100644 --- a/examples/games/contributors.rs +++ b/examples/games/contributors.rs @@ -334,7 +334,7 @@ fn contributors() -> Result { let contributors = BufReader::new(stdout) .lines() - .filter_map(|x| x.ok()) + .map_while(|x| x.ok()) .collect(); Ok(contributors) diff --git a/examples/stress_tests/bevymark.rs b/examples/stress_tests/bevymark.rs index 4518b804e7..5bb09e205b 100644 --- a/examples/stress_tests/bevymark.rs +++ b/examples/stress_tests/bevymark.rs @@ -37,7 +37,7 @@ fn main() { }), ..default() })) - .add_plugin(FrameTimeDiagnosticsPlugin::default()) + .add_plugin(FrameTimeDiagnosticsPlugin) .add_plugin(LogDiagnosticsPlugin::default()) .insert_resource(BevyCounter { count: 0, diff --git a/examples/stress_tests/many_animated_sprites.rs b/examples/stress_tests/many_animated_sprites.rs index e6286c39c7..a5dd281de7 100644 --- a/examples/stress_tests/many_animated_sprites.rs +++ b/examples/stress_tests/many_animated_sprites.rs @@ -21,7 +21,7 @@ fn main() { App::new() // Since this is also used as a benchmark, we want it to display performance data. .add_plugin(LogDiagnosticsPlugin::default()) - .add_plugin(FrameTimeDiagnosticsPlugin::default()) + .add_plugin(FrameTimeDiagnosticsPlugin) .add_plugins(DefaultPlugins.set(WindowPlugin { primary_window: Some(Window { present_mode: PresentMode::AutoNoVsync, diff --git a/examples/stress_tests/many_buttons.rs b/examples/stress_tests/many_buttons.rs index 94a7ddc951..19d424a1df 100644 --- a/examples/stress_tests/many_buttons.rs +++ b/examples/stress_tests/many_buttons.rs @@ -30,7 +30,7 @@ fn main() { }), ..default() })) - .add_plugin(FrameTimeDiagnosticsPlugin::default()) + .add_plugin(FrameTimeDiagnosticsPlugin) .add_plugin(LogDiagnosticsPlugin::default()) .add_systems(Startup, setup) .add_systems(Update, button_system); diff --git a/examples/stress_tests/many_cubes.rs b/examples/stress_tests/many_cubes.rs index 6d0d7db3f1..dc20c4fa36 100644 --- a/examples/stress_tests/many_cubes.rs +++ b/examples/stress_tests/many_cubes.rs @@ -28,7 +28,7 @@ fn main() { }), ..default() })) - .add_plugin(FrameTimeDiagnosticsPlugin::default()) + .add_plugin(FrameTimeDiagnosticsPlugin) .add_plugin(LogDiagnosticsPlugin::default()) .add_systems(Startup, setup) .add_systems(Update, (move_camera, print_mesh_count)) diff --git a/examples/stress_tests/many_gizmos.rs b/examples/stress_tests/many_gizmos.rs index 817a2e8aae..dc60b40977 100644 --- a/examples/stress_tests/many_gizmos.rs +++ b/examples/stress_tests/many_gizmos.rs @@ -18,7 +18,7 @@ fn main() { }), ..default() })) - .add_plugin(FrameTimeDiagnosticsPlugin::default()) + .add_plugin(FrameTimeDiagnosticsPlugin) .insert_resource(Config { line_count: 50_000, fancy: false, diff --git a/examples/stress_tests/many_glyphs.rs b/examples/stress_tests/many_glyphs.rs index 73aa78c08e..77127b57ca 100644 --- a/examples/stress_tests/many_glyphs.rs +++ b/examples/stress_tests/many_glyphs.rs @@ -21,7 +21,7 @@ fn main() { }), ..default() })) - .add_plugin(FrameTimeDiagnosticsPlugin::default()) + .add_plugin(FrameTimeDiagnosticsPlugin) .add_plugin(LogDiagnosticsPlugin::default()) .add_systems(Startup, setup); diff --git a/examples/stress_tests/many_lights.rs b/examples/stress_tests/many_lights.rs index c00e8c6cf5..bf81668536 100644 --- a/examples/stress_tests/many_lights.rs +++ b/examples/stress_tests/many_lights.rs @@ -24,7 +24,7 @@ fn main() { }), ..default() })) - .add_plugin(FrameTimeDiagnosticsPlugin::default()) + .add_plugin(FrameTimeDiagnosticsPlugin) .add_plugin(LogDiagnosticsPlugin::default()) .add_systems(Startup, setup) .add_systems(Update, (move_camera, print_light_count)) diff --git a/examples/stress_tests/many_sprites.rs b/examples/stress_tests/many_sprites.rs index bc8423d86d..87776ef61e 100644 --- a/examples/stress_tests/many_sprites.rs +++ b/examples/stress_tests/many_sprites.rs @@ -29,7 +29,7 @@ fn main() { )) // Since this is also used as a benchmark, we want it to display performance data. .add_plugin(LogDiagnosticsPlugin::default()) - .add_plugin(FrameTimeDiagnosticsPlugin::default()) + .add_plugin(FrameTimeDiagnosticsPlugin) .add_plugins(DefaultPlugins.set(WindowPlugin { primary_window: Some(Window { present_mode: PresentMode::AutoNoVsync, diff --git a/examples/stress_tests/text_pipeline.rs b/examples/stress_tests/text_pipeline.rs index 9eaaeedc1d..64ffe57ae7 100644 --- a/examples/stress_tests/text_pipeline.rs +++ b/examples/stress_tests/text_pipeline.rs @@ -18,7 +18,7 @@ fn main() { }), ..default() })) - .add_plugin(FrameTimeDiagnosticsPlugin::default()) + .add_plugin(FrameTimeDiagnosticsPlugin) .add_plugin(LogDiagnosticsPlugin::default()) .add_systems(Startup, spawn) .add_systems(Update, update_text_bounds) diff --git a/examples/stress_tests/transform_hierarchy.rs b/examples/stress_tests/transform_hierarchy.rs index 47985fbbf6..18562fcd0d 100644 --- a/examples/stress_tests/transform_hierarchy.rs +++ b/examples/stress_tests/transform_hierarchy.rs @@ -184,7 +184,7 @@ fn main() { App::new() .insert_resource(cfg) .add_plugins(MinimalPlugins) - .add_plugin(TransformPlugin::default()) + .add_plugin(TransformPlugin) .add_systems(Startup, setup) // Updating transforms *must* be done before `CoreSet::PostUpdate` // or the hierarchy will momentarily be in an invalid state. diff --git a/examples/ui/text.rs b/examples/ui/text.rs index 0976e0f445..f6c5b82fff 100644 --- a/examples/ui/text.rs +++ b/examples/ui/text.rs @@ -11,7 +11,7 @@ use bevy::{ fn main() { App::new() .add_plugins(DefaultPlugins) - .add_plugin(FrameTimeDiagnosticsPlugin::default()) + .add_plugin(FrameTimeDiagnosticsPlugin) .add_systems(Startup, setup) .add_systems(Update, (text_update_system, text_color_system)) .run(); diff --git a/examples/window/window_resizing.rs b/examples/window/window_resizing.rs index 6f130f5fa5..5281454765 100644 --- a/examples/window/window_resizing.rs +++ b/examples/window/window_resizing.rs @@ -1,4 +1,4 @@ -///! This example illustrates how to resize windows, and how to respond to a window being resized. +//! This example illustrates how to resize windows, and how to respond to a window being resized. use bevy::{prelude::*, window::WindowResized}; fn main() { From 4465f256eba401fff838173993e1bfc792da5034 Mon Sep 17 00:00:00 2001 From: Marco Buono Date: Mon, 29 May 2023 12:15:01 -0300 Subject: [PATCH 03/15] Add `MAY_DISCARD` shader def, enabling early depth tests for most cases (#6697) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Objective - Right now we can't really benefit from [early depth testing](https://www.khronos.org/opengl/wiki/Early_Fragment_Test) in our PBR shader because it includes codepaths with `discard`, even for situations where they are not necessary. ## Solution - This PR introduces a new `MeshPipelineKey` and shader def, `MAY_DISCARD`; - All possible material/mesh options that that may result in `discard`s being needed must set `MAY_DISCARD` ahead of time: - Right now, this is only `AlphaMode::Mask(f32)`, but in the future might include other options/effects; (e.g. one effect I'm personally interested in is bayer dither pseudo-transparency for LOD transitions of opaque meshes) - Shader codepaths that can `discard` are guarded by an `#ifdef MAY_DISCARD` preprocessor directive: - Right now, this is just one branch in `alpha_discard()`; - If `MAY_DISCARD` is _not_ set, the `@early_depth_test` attribute is added to the PBR fragment shader. This is a not yet documented, possibly non-standard WGSL extension I found browsing Naga's source code. [I opened a PR to document it there](https://github.com/gfx-rs/naga/pull/2132). My understanding is that for backends where this attribute is supported, it will force an explicit opt-in to early depth test. (e.g. via `layout(early_fragment_tests) in;` in GLSL) ## Caveats - I included `@early_depth_test` for the sake of us being explicit, and avoiding the need for the driver to be “smart” about enabling this feature. That way, if we make a mistake and include a `discard` unguarded by `MAY_DISCARD`, it will either produce errors or noticeable visual artifacts so that we'll catch early, instead of causing a performance regression. - I'm not sure explicit early depth test is supported on the naga Metal backend, which is what I'm currently using, so I can't really test the explicit early depth test enable, I would like others with Vulkan/GL hardware to test it if possible; - I would like some guidance on how to measure/verify the performance benefits of this; - If I understand it correctly, this, or _something like this_ is needed to fully reap the performance gains enabled by #6284; - This will _most definitely_ conflict with #6284 and #6644. I can fix the conflicts as needed, depending on whether/the order they end up being merging in. --- ## Changelog ### Changed - Early depth tests are now enabled whenever possible for meshes using `StandardMaterial`, reducing the number of fragments evaluated for scenes with lots of occlusions. --- crates/bevy_pbr/src/material.rs | 3 ++ crates/bevy_pbr/src/prepass/mod.rs | 10 ++--- crates/bevy_pbr/src/render/light.rs | 10 ++--- crates/bevy_pbr/src/render/mesh.rs | 7 +++- crates/bevy_pbr/src/render/pbr_functions.wgsl | 10 +++-- crates/bevy_pbr/src/render/pbr_prepass.wgsl | 40 +++++++------------ 6 files changed, 39 insertions(+), 41 deletions(-) diff --git a/crates/bevy_pbr/src/material.rs b/crates/bevy_pbr/src/material.rs index 19c5349df3..e9dbce227e 100644 --- a/crates/bevy_pbr/src/material.rs +++ b/crates/bevy_pbr/src/material.rs @@ -472,6 +472,9 @@ pub fn queue_material_meshes( AlphaMode::Multiply => { mesh_key |= MeshPipelineKey::BLEND_MULTIPLY; } + AlphaMode::Mask(_) => { + mesh_key |= MeshPipelineKey::MAY_DISCARD; + } _ => (), } diff --git a/crates/bevy_pbr/src/prepass/mod.rs b/crates/bevy_pbr/src/prepass/mod.rs index 7fd87d4716..9307bc0d75 100644 --- a/crates/bevy_pbr/src/prepass/mod.rs +++ b/crates/bevy_pbr/src/prepass/mod.rs @@ -364,8 +364,8 @@ where shader_defs.push("DEPTH_PREPASS".into()); } - if key.mesh_key.contains(MeshPipelineKey::ALPHA_MASK) { - shader_defs.push("ALPHA_MASK".into()); + if key.mesh_key.contains(MeshPipelineKey::MAY_DISCARD) { + shader_defs.push("MAY_DISCARD".into()); } let blend_key = key @@ -467,9 +467,7 @@ where // is enabled or the material uses alpha cutoff values and doesn't rely on the standard // prepass shader let fragment_required = !targets.is_empty() - || ((key.mesh_key.contains(MeshPipelineKey::ALPHA_MASK) - || blend_key == MeshPipelineKey::BLEND_PREMULTIPLIED_ALPHA - || blend_key == MeshPipelineKey::BLEND_ALPHA) + || (key.mesh_key.contains(MeshPipelineKey::MAY_DISCARD) && self.material_fragment_shader.is_some()); let fragment = fragment_required.then(|| { @@ -967,7 +965,7 @@ pub fn queue_prepass_material_meshes( let alpha_mode = material.properties.alpha_mode; match alpha_mode { AlphaMode::Opaque => {} - AlphaMode::Mask(_) => mesh_key |= MeshPipelineKey::ALPHA_MASK, + AlphaMode::Mask(_) => mesh_key |= MeshPipelineKey::MAY_DISCARD, AlphaMode::Blend | AlphaMode::Premultiplied | AlphaMode::Add diff --git a/crates/bevy_pbr/src/render/light.rs b/crates/bevy_pbr/src/render/light.rs index 410ec24471..85dacdad9b 100644 --- a/crates/bevy_pbr/src/render/light.rs +++ b/crates/bevy_pbr/src/render/light.rs @@ -1608,11 +1608,11 @@ pub fn queue_shadows( } let alpha_mode = material.properties.alpha_mode; match alpha_mode { - AlphaMode::Mask(_) => { - mesh_key |= MeshPipelineKey::ALPHA_MASK; - } - AlphaMode::Blend | AlphaMode::Premultiplied | AlphaMode::Add => { - mesh_key |= MeshPipelineKey::BLEND_PREMULTIPLIED_ALPHA; + AlphaMode::Mask(_) + | AlphaMode::Blend + | AlphaMode::Premultiplied + | AlphaMode::Add => { + mesh_key |= MeshPipelineKey::MAY_DISCARD; } _ => {} } diff --git a/crates/bevy_pbr/src/render/mesh.rs b/crates/bevy_pbr/src/render/mesh.rs index 3224c1275e..3a3834a813 100644 --- a/crates/bevy_pbr/src/render/mesh.rs +++ b/crates/bevy_pbr/src/render/mesh.rs @@ -582,7 +582,8 @@ bitflags::bitflags! { const DEPTH_PREPASS = (1 << 3); const NORMAL_PREPASS = (1 << 4); const MOTION_VECTOR_PREPASS = (1 << 5); - const ALPHA_MASK = (1 << 6); + const MAY_DISCARD = (1 << 6); // Guards shader codepaths that may discard, allowing early depth tests in most cases + // See: https://www.khronos.org/opengl/wiki/Early_Fragment_Test const ENVIRONMENT_MAP = (1 << 7); const DEPTH_CLAMP_ORTHO = (1 << 8); const BLEND_RESERVED_BITS = Self::BLEND_MASK_BITS << Self::BLEND_SHIFT_BITS; // ← Bitmask reserving bits for the blend state @@ -795,6 +796,10 @@ impl SpecializedMeshPipeline for MeshPipeline { } } + if key.contains(MeshPipelineKey::MAY_DISCARD) { + shader_defs.push("MAY_DISCARD".into()); + } + if key.contains(MeshPipelineKey::ENVIRONMENT_MAP) { shader_defs.push("ENVIRONMENT_MAP".into()); } diff --git a/crates/bevy_pbr/src/render/pbr_functions.wgsl b/crates/bevy_pbr/src/render/pbr_functions.wgsl index 1bc782fc19..56e3d17773 100644 --- a/crates/bevy_pbr/src/render/pbr_functions.wgsl +++ b/crates/bevy_pbr/src/render/pbr_functions.wgsl @@ -14,16 +14,20 @@ fn alpha_discard(material: StandardMaterial, output_color: vec4) -> vec4= material.alpha_cutoff { // NOTE: If rendering as masked alpha and >= the cutoff, render as fully opaque color.a = 1.0; } else { - // NOTE: output_color.a < in.material.alpha_cutoff should not is not rendered - // NOTE: This and any other discards mean that early-z testing cannot be done! + // NOTE: output_color.a < in.material.alpha_cutoff should not be rendered discard; } } +#endif + return color; } diff --git a/crates/bevy_pbr/src/render/pbr_prepass.wgsl b/crates/bevy_pbr/src/render/pbr_prepass.wgsl index d96a23b845..1a90c45709 100644 --- a/crates/bevy_pbr/src/render/pbr_prepass.wgsl +++ b/crates/bevy_pbr/src/render/pbr_prepass.wgsl @@ -30,19 +30,7 @@ const PREMULTIPLIED_ALPHA_CUTOFF = 0.05; // We can use a simplified version of alpha_discard() here since we only need to handle the alpha_cutoff fn prepass_alpha_discard(in: FragmentInput) { -// This is a workaround since the preprocessor does not support -// #if defined(ALPHA_MASK) || defined(BLEND_PREMULTIPLIED_ALPHA) -#ifndef ALPHA_MASK -#ifndef BLEND_PREMULTIPLIED_ALPHA -#ifndef BLEND_ALPHA - -#define EMPTY_PREPASS_ALPHA_DISCARD - -#endif // BLEND_ALPHA -#endif // BLEND_PREMULTIPLIED_ALPHA not defined -#endif // ALPHA_MASK not defined - -#ifndef EMPTY_PREPASS_ALPHA_DISCARD +#ifdef MAY_DISCARD var output_color: vec4 = material.base_color; #ifdef VERTEX_UVS @@ -51,22 +39,22 @@ fn prepass_alpha_discard(in: FragmentInput) { } #endif // VERTEX_UVS -#ifdef ALPHA_MASK - if ((material.flags & STANDARD_MATERIAL_FLAGS_ALPHA_MODE_MASK) != 0u) && output_color.a < material.alpha_cutoff { - discard; - } -#else // BLEND_PREMULTIPLIED_ALPHA || BLEND_ALPHA let alpha_mode = material.flags & STANDARD_MATERIAL_FLAGS_ALPHA_MODE_RESERVED_BITS; - if (alpha_mode == STANDARD_MATERIAL_FLAGS_ALPHA_MODE_BLEND || alpha_mode == STANDARD_MATERIAL_FLAGS_ALPHA_MODE_ADD) - && output_color.a < PREMULTIPLIED_ALPHA_CUTOFF { - discard; - } else if alpha_mode == STANDARD_MATERIAL_FLAGS_ALPHA_MODE_PREMULTIPLIED - && all(output_color < vec4(PREMULTIPLIED_ALPHA_CUTOFF)) { - discard; + if alpha_mode == STANDARD_MATERIAL_FLAGS_ALPHA_MODE_MASK { + if output_color.a < material.alpha_cutoff { + discard; + } + } else if (alpha_mode == STANDARD_MATERIAL_FLAGS_ALPHA_MODE_BLEND || alpha_mode == STANDARD_MATERIAL_FLAGS_ALPHA_MODE_ADD) { + if output_color.a < PREMULTIPLIED_ALPHA_CUTOFF { + discard; + } + } else if alpha_mode == STANDARD_MATERIAL_FLAGS_ALPHA_MODE_PREMULTIPLIED { + if all(output_color < vec4(PREMULTIPLIED_ALPHA_CUTOFF)) { + discard; + } } -#endif // !ALPHA_MASK -#endif // EMPTY_PREPASS_ALPHA_DISCARD not defined +#endif // MAY_DISCARD } #ifdef PREPASS_FRAGMENT From 85a918a8dded4844b34dc22d82bf1ca3c485bcb9 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 29 May 2023 11:22:10 -0400 Subject: [PATCH 04/15] Improve safety for the multi-threaded executor using `UnsafeWorldCell` (#8292) # Objective Fix #7833. Safety comments in the multi-threaded executor don't really talk about system world accesses, which makes it unclear if the code is actually valid. ## Solution Update the `System` trait to use `UnsafeWorldCell`. This type's API is written in a way that makes it much easier to cleanly maintain safety invariants. Use this type throughout the multi-threaded executor, with a liberal use of safety comments. --- ## Migration Guide The `System` trait now uses `UnsafeWorldCell` instead of `&World`. This type provides a robust API for interior mutable world access. - The method `run_unsafe` uses this type to manage world mutations across multiple threads. - The method `update_archetype_component_access` uses this type to ensure that only world metadata can be used. ```rust let mut system = IntoSystem::into_system(my_system); system.initialize(&mut world); // Before: system.update_archetype_component_access(&world); unsafe { system.run_unsafe(&world) } // After: system.update_archetype_component_access(world.as_unsafe_world_cell_readonly()); unsafe { system.run_unsafe(world.as_unsafe_world_cell()) } ``` --------- Co-authored-by: James Liu --- .../bevy_ecs/iteration/heavy_compute.rs | 2 +- .../bevy_ecs/iteration/iter_simple_system.rs | 2 +- benches/benches/bevy_ecs/world/world_get.rs | 2 +- crates/bevy_ecs/src/schedule/condition.rs | 5 +- .../src/schedule/executor/multi_threaded.rs | 89 ++++++++++++------- crates/bevy_ecs/src/system/combinator.rs | 5 +- .../src/system/exclusive_function_system.rs | 6 +- crates/bevy_ecs/src/system/function_system.rs | 8 +- crates/bevy_ecs/src/system/mod.rs | 6 +- crates/bevy_ecs/src/system/system.rs | 31 ++++--- crates/bevy_ecs/src/world/identifier.rs | 6 +- .../bevy_ecs/src/world/unsafe_world_cell.rs | 10 ++- 12 files changed, 103 insertions(+), 69 deletions(-) diff --git a/benches/benches/bevy_ecs/iteration/heavy_compute.rs b/benches/benches/bevy_ecs/iteration/heavy_compute.rs index 99b3fdb764..9795c82159 100644 --- a/benches/benches/bevy_ecs/iteration/heavy_compute.rs +++ b/benches/benches/bevy_ecs/iteration/heavy_compute.rs @@ -45,7 +45,7 @@ pub fn heavy_compute(c: &mut Criterion) { let mut system = IntoSystem::into_system(sys); system.initialize(&mut world); - system.update_archetype_component_access(&world); + system.update_archetype_component_access(world.as_unsafe_world_cell()); b.iter(move || system.run((), &mut world)); }); diff --git a/benches/benches/bevy_ecs/iteration/iter_simple_system.rs b/benches/benches/bevy_ecs/iteration/iter_simple_system.rs index fc90878c58..2b09ada53e 100644 --- a/benches/benches/bevy_ecs/iteration/iter_simple_system.rs +++ b/benches/benches/bevy_ecs/iteration/iter_simple_system.rs @@ -37,7 +37,7 @@ impl Benchmark { let mut system = IntoSystem::into_system(query_system); system.initialize(&mut world); - system.update_archetype_component_access(&world); + system.update_archetype_component_access(world.as_unsafe_world_cell()); Self(world, Box::new(system)) } diff --git a/benches/benches/bevy_ecs/world/world_get.rs b/benches/benches/bevy_ecs/world/world_get.rs index 4fc7ed5104..ee63498a09 100644 --- a/benches/benches/bevy_ecs/world/world_get.rs +++ b/benches/benches/bevy_ecs/world/world_get.rs @@ -291,7 +291,7 @@ pub fn query_get_component_simple(criterion: &mut Criterion) { let mut system = IntoSystem::into_system(query_system); system.initialize(&mut world); - system.update_archetype_component_access(&world); + system.update_archetype_component_access(world.as_unsafe_world_cell()); bencher.iter(|| system.run(entity, &mut world)); }); diff --git a/crates/bevy_ecs/src/schedule/condition.rs b/crates/bevy_ecs/src/schedule/condition.rs index 24d80040cf..52630acc2b 100644 --- a/crates/bevy_ecs/src/schedule/condition.rs +++ b/crates/bevy_ecs/src/schedule/condition.rs @@ -5,6 +5,7 @@ use std::ops::Not; use crate::component::{self, ComponentId}; use crate::query::Access; use crate::system::{CombinatorSystem, Combine, IntoSystem, ReadOnlySystem, System}; +use crate::world::unsafe_world_cell::UnsafeWorldCell; use crate::world::World; pub type BoxedCondition = Box>; @@ -990,7 +991,7 @@ where self.condition.is_exclusive() } - unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out { + unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out { // SAFETY: The inner condition system asserts its own safety. !self.condition.run_unsafe(input, world) } @@ -1007,7 +1008,7 @@ where self.condition.initialize(world); } - fn update_archetype_component_access(&mut self, world: &World) { + fn update_archetype_component_access(&mut self, world: UnsafeWorldCell) { self.condition.update_archetype_component_access(world); } diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index f6ba57e532..569320c031 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -21,7 +21,7 @@ use crate::{ is_apply_system_buffers, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule, }, system::BoxedSystem, - world::World, + world::{unsafe_world_cell::UnsafeWorldCell, World}, }; use crate as bevy_ecs; @@ -184,7 +184,6 @@ impl SystemExecutor for MultiThreadedExecutor { .map(|e| e.0.clone()); let thread_executor = thread_executor.as_deref(); - let world = SyncUnsafeCell::from_mut(world); let SyncUnsafeSchedule { systems, mut conditions, @@ -197,10 +196,13 @@ impl SystemExecutor for MultiThreadedExecutor { // the executor itself is a `Send` future so that it can run // alongside systems that claim the local thread let executor = async { + let world_cell = world.as_unsafe_world_cell(); while self.num_completed_systems < self.num_systems { - // SAFETY: self.ready_systems does not contain running systems + // SAFETY: + // - self.ready_systems does not contain running systems. + // - `world_cell` has mutable access to the entire world. unsafe { - self.spawn_system_tasks(scope, systems, &mut conditions, world); + self.spawn_system_tasks(scope, systems, &mut conditions, world_cell); } if self.num_running_systems > 0 { @@ -231,7 +233,7 @@ impl SystemExecutor for MultiThreadedExecutor { if self.apply_final_buffers { // Do one final apply buffers after all systems have completed // Commands should be applied while on the scope's thread, not the executor's thread - let res = apply_system_buffers(&self.unapplied_systems, systems, world.get_mut()); + let res = apply_system_buffers(&self.unapplied_systems, systems, world); if let Err(payload) = res { let mut panic_payload = self.panic_payload.lock().unwrap(); *panic_payload = Some(payload); @@ -283,14 +285,16 @@ impl MultiThreadedExecutor { } /// # Safety - /// Caller must ensure that `self.ready_systems` does not contain any systems that - /// have been mutably borrowed (such as the systems currently running). + /// - Caller must ensure that `self.ready_systems` does not contain any systems that + /// have been mutably borrowed (such as the systems currently running). + /// - `world_cell` must have permission to access all world data (not counting + /// any world data that is claimed by systems currently running on this executor). unsafe fn spawn_system_tasks<'scope>( &mut self, scope: &Scope<'_, 'scope, ()>, systems: &'scope [SyncUnsafeCell], conditions: &mut Conditions, - cell: &'scope SyncUnsafeCell, + world_cell: UnsafeWorldCell<'scope>, ) { if self.exclusive_running { return; @@ -307,10 +311,7 @@ impl MultiThreadedExecutor { // Therefore, no other reference to this system exists and there is no aliasing. let system = unsafe { &mut *systems[system_index].get() }; - // SAFETY: No exclusive system is running. - // Therefore, there is no existing mutable reference to the world. - let world = unsafe { &*cell.get() }; - if !self.can_run(system_index, system, conditions, world) { + if !self.can_run(system_index, system, conditions, world_cell) { // NOTE: exclusive systems with ambiguities are susceptible to // being significantly displaced here (compared to single-threaded order) // if systems after them in topological order can run @@ -320,9 +321,10 @@ impl MultiThreadedExecutor { self.ready_systems.set(system_index, false); - // SAFETY: Since `self.can_run` returned true earlier, it must have called - // `update_archetype_component_access` for each run condition. - if !self.should_run(system_index, system, conditions, world) { + // SAFETY: `can_run` returned true, which means that: + // - It must have called `update_archetype_component_access` for each run condition. + // - There can be no systems running whose accesses would conflict with any conditions. + if !self.should_run(system_index, system, conditions, world_cell) { self.skip_system_and_signal_dependents(system_index); continue; } @@ -331,10 +333,12 @@ impl MultiThreadedExecutor { self.num_running_systems += 1; if self.system_task_metadata[system_index].is_exclusive { - // SAFETY: `can_run` confirmed that no systems are running. - // Therefore, there is no existing reference to the world. + // SAFETY: `can_run` returned true for this system, which means + // that no other systems currently have access to the world. + let world = unsafe { world_cell.world_mut() }; + // SAFETY: `can_run` returned true for this system, + // which means no systems are currently borrowed. unsafe { - let world = &mut *cell.get(); self.spawn_exclusive_system_task(scope, system_index, systems, world); } break; @@ -342,9 +346,10 @@ impl MultiThreadedExecutor { // SAFETY: // - No other reference to this system exists. - // - `self.can_run` has been called, which calls `update_archetype_component_access` with this system. + // - `can_run` has been called, which calls `update_archetype_component_access` with this system. + // - `can_run` returned true, so no systems with conflicting world access are running. unsafe { - self.spawn_system_task(scope, system_index, systems, world); + self.spawn_system_task(scope, system_index, systems, world_cell); } } @@ -357,7 +362,7 @@ impl MultiThreadedExecutor { system_index: usize, system: &mut BoxedSystem, conditions: &mut Conditions, - world: &World, + world: UnsafeWorldCell, ) -> bool { let system_meta = &self.system_task_metadata[system_index]; if system_meta.is_exclusive && self.num_running_systems > 0 { @@ -413,15 +418,17 @@ impl MultiThreadedExecutor { } /// # Safety - /// - /// `update_archetype_component` must have been called with `world` - /// for each run condition in `conditions`. + /// * `world` must have permission to read any world data required by + /// the system's conditions: this includes conditions for the system + /// itself, and conditions for any of the system's sets. + /// * `update_archetype_component` must have been called with `world` + /// for each run condition in `conditions`. unsafe fn should_run( &mut self, system_index: usize, _system: &BoxedSystem, conditions: &mut Conditions, - world: &World, + world: UnsafeWorldCell, ) -> bool { let mut should_run = !self.skipped_systems.contains(system_index); for set_idx in conditions.sets_with_conditions_of_systems[system_index].ones() { @@ -430,7 +437,10 @@ impl MultiThreadedExecutor { } // Evaluate the system set's conditions. - // SAFETY: `update_archetype_component_access` has been called for each run condition. + // SAFETY: + // - The caller ensures that `world` has permission to read any data + // required by the conditions. + // - `update_archetype_component_access` has been called for each run condition. let set_conditions_met = evaluate_and_fold_conditions(&mut conditions.set_conditions[set_idx], world); @@ -444,7 +454,10 @@ impl MultiThreadedExecutor { } // Evaluate the system's conditions. - // SAFETY: `update_archetype_component_access` has been called for each run condition. + // SAFETY: + // - The caller ensures that `world` has permission to read any data + // required by the conditions. + // - `update_archetype_component_access` has been called for each run condition. let system_conditions_met = evaluate_and_fold_conditions(&mut conditions.system_conditions[system_index], world); @@ -459,6 +472,8 @@ impl MultiThreadedExecutor { /// # Safety /// - Caller must not alias systems that are running. + /// - `world` must have permission to access the world data + /// used by the specified system. /// - `update_archetype_component_access` must have been called with `world` /// on the system assocaited with `system_index`. unsafe fn spawn_system_task<'scope>( @@ -466,7 +481,7 @@ impl MultiThreadedExecutor { scope: &Scope<'_, 'scope, ()>, system_index: usize, systems: &'scope [SyncUnsafeCell], - world: &'scope World, + world: UnsafeWorldCell<'scope>, ) { // SAFETY: this system is not running, no other reference exists let system = unsafe { &mut *systems[system_index].get() }; @@ -483,7 +498,8 @@ impl MultiThreadedExecutor { let system_guard = system_span.enter(); let res = std::panic::catch_unwind(AssertUnwindSafe(|| { // SAFETY: - // - Access: TODO. + // - The caller ensures that we have permission to + // access the world data used by the system. // - `update_archetype_component_access` has been called. unsafe { system.run_unsafe((), world) }; })); @@ -688,10 +704,14 @@ fn apply_system_buffers( } /// # Safety -/// -/// `update_archetype_component_access` must have been called -/// with `world` for each condition in `conditions`. -unsafe fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &World) -> bool { +/// - `world` must have permission to read any world data +/// required by `conditions`. +/// - `update_archetype_component_access` must have been called +/// with `world` for each condition in `conditions`. +unsafe fn evaluate_and_fold_conditions( + conditions: &mut [BoxedCondition], + world: UnsafeWorldCell, +) -> bool { // not short-circuiting is intentional #[allow(clippy::unnecessary_fold)] conditions @@ -699,7 +719,8 @@ unsafe fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: .map(|condition| { #[cfg(feature = "trace")] let _condition_span = info_span!("condition", name = &*condition.name()).entered(); - // SAFETY: caller ensures system access is compatible + // SAFETY: The caller ensures that `world` has permission to + // access any data required by the condition. unsafe { condition.run_unsafe((), world) } }) .fold(true, |acc, res| acc && res) diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index 32ca444137..c058a78f35 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/crates/bevy_ecs/src/system/combinator.rs @@ -7,6 +7,7 @@ use crate::{ component::{ComponentId, Tick}, prelude::World, query::Access, + world::unsafe_world_cell::UnsafeWorldCell, }; use super::{ReadOnlySystem, System}; @@ -157,7 +158,7 @@ where self.a.is_exclusive() || self.b.is_exclusive() } - unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out { + unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out { Func::combine( input, // SAFETY: The world accesses for both underlying systems have been registered, @@ -198,7 +199,7 @@ where self.component_access.extend(self.b.component_access()); } - fn update_archetype_component_access(&mut self, world: &World) { + fn update_archetype_component_access(&mut self, world: UnsafeWorldCell) { self.a.update_archetype_component_access(world); self.b.update_archetype_component_access(world); diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index c1c4bff3a2..9ba43b4015 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -6,7 +6,7 @@ use crate::{ check_system_change_tick, ExclusiveSystemParam, ExclusiveSystemParamItem, In, IntoSystem, System, SystemMeta, }, - world::World, + world::{unsafe_world_cell::UnsafeWorldCell, World}, }; use bevy_utils::all_tuples; @@ -86,7 +86,7 @@ where } #[inline] - unsafe fn run_unsafe(&mut self, _input: Self::In, _world: &World) -> Self::Out { + unsafe fn run_unsafe(&mut self, _input: Self::In, _world: UnsafeWorldCell) -> Self::Out { panic!("Cannot run exclusive systems with a shared World reference"); } @@ -134,7 +134,7 @@ where self.param_state = Some(F::Param::init(world, &mut self.system_meta)); } - fn update_archetype_component_access(&mut self, _world: &World) {} + fn update_archetype_component_access(&mut self, _world: UnsafeWorldCell) {} #[inline] fn check_change_tick(&mut self, change_tick: Tick) { diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 27593cfef9..ed73d75b8f 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -4,7 +4,7 @@ use crate::{ prelude::FromWorld, query::{Access, FilteredAccessSet}, system::{check_system_change_tick, ReadOnlySystemParam, System, SystemParam, SystemParamItem}, - world::{World, WorldId}, + world::{unsafe_world_cell::UnsafeWorldCell, World, WorldId}, }; use bevy_utils::all_tuples; @@ -417,7 +417,7 @@ where } #[inline] - unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out { + unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out { let change_tick = world.increment_change_tick(); // SAFETY: @@ -428,7 +428,7 @@ where let params = F::Param::get_param( self.param_state.as_mut().expect(Self::PARAM_MESSAGE), &self.system_meta, - world.as_unsafe_world_cell_migration_internal(), + world, change_tick, ); let out = self.func.run(input, params); @@ -457,7 +457,7 @@ where self.param_state = Some(F::Param::init_state(world, &mut self.system_meta)); } - fn update_archetype_component_access(&mut self, world: &World) { + fn update_archetype_component_access(&mut self, world: UnsafeWorldCell) { assert!(self.world_id == Some(world.id()), "Encountered a mismatched World. A System cannot be used with Worlds other than the one it was initialized with."); let archetypes = world.archetypes(); let new_generation = archetypes.generation(); diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index c322e16861..9bb99138da 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -1610,7 +1610,7 @@ mod tests { // set up system and verify its access is empty system.initialize(&mut world); - system.update_archetype_component_access(&world); + system.update_archetype_component_access(world.as_unsafe_world_cell()); assert_eq!( system .archetype_component_access() @@ -1640,7 +1640,7 @@ mod tests { world.spawn((B, C)); // update system and verify its accesses are correct - system.update_archetype_component_access(&world); + system.update_archetype_component_access(world.as_unsafe_world_cell()); assert_eq!( system .archetype_component_access() @@ -1658,7 +1658,7 @@ mod tests { .unwrap(), ); world.spawn((A, B, D)); - system.update_archetype_component_access(&world); + system.update_archetype_component_access(world.as_unsafe_world_cell()); assert_eq!( system .archetype_component_access() diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index ae4a9fc074..4a578ec679 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -2,6 +2,7 @@ use bevy_utils::tracing::warn; use core::fmt::Debug; use crate::component::Tick; +use crate::world::unsafe_world_cell::UnsafeWorldCell; use crate::{archetype::ArchetypeComponentId, component::ComponentId, query::Access, world::World}; use std::any::TypeId; @@ -39,26 +40,24 @@ pub trait System: Send + Sync + 'static { fn is_exclusive(&self) -> bool; /// Runs the system with the given input in the world. Unlike [`System::run`], this function - /// takes a shared reference to [`World`] and may therefore break Rust's aliasing rules, making - /// it unsafe to call. + /// can be called in parallel with other systems and may break Rust's aliasing rules + /// if used incorrectly, making it unsafe to call. /// /// # Safety /// - /// This might access world and resources in an unsafe manner. This should only be called in one - /// of the following contexts: - /// 1. This system is the only system running on the given world across all threads. - /// 2. This system only runs in parallel with other systems that do not conflict with the - /// [`System::archetype_component_access()`]. - /// - /// Additionally, the method [`Self::update_archetype_component_access`] must be called at some - /// point before this one, with the same exact [`World`]. If `update_archetype_component_access` - /// panics (or otherwise does not return for any reason), this method must not be called. - unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out; + /// - The caller must ensure that `world` has permission to access any world data + /// registered in [`Self::archetype_component_access`]. There must be no conflicting + /// simultaneous accesses while the system is running. + /// - The method [`Self::update_archetype_component_access`] must be called at some + /// point before this one, with the same exact [`World`]. If `update_archetype_component_access` + /// panics (or otherwise does not return for any reason), this method must not be called. + unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out; /// Runs the system with the given input in the world. fn run(&mut self, input: Self::In, world: &mut World) -> Self::Out { + let world = world.as_unsafe_world_cell(); self.update_archetype_component_access(world); // SAFETY: - // - World and resources are exclusively borrowed, which ensures no data access conflicts. + // - We have exclusive access to the entire world. // - `update_archetype_component_access` has been called. unsafe { self.run_unsafe(input, world) } } @@ -66,7 +65,11 @@ pub trait System: Send + Sync + 'static { /// Initialize the system. fn initialize(&mut self, _world: &mut World); /// Update the system's archetype component [`Access`]. - fn update_archetype_component_access(&mut self, world: &World); + /// + /// ## Note for implementors + /// `world` may only be used to access metadata. This can be done in safe code + /// via functions such as [`UnsafeWorldCell::archetypes`]. + fn update_archetype_component_access(&mut self, world: UnsafeWorldCell); fn check_change_tick(&mut self, change_tick: Tick); /// Returns the system's default [system sets](crate::schedule::SystemSet). fn default_system_sets(&self) -> Vec> { diff --git a/crates/bevy_ecs/src/world/identifier.rs b/crates/bevy_ecs/src/world/identifier.rs index e5f8d7fde7..626964577d 100644 --- a/crates/bevy_ecs/src/world/identifier.rs +++ b/crates/bevy_ecs/src/world/identifier.rs @@ -44,10 +44,10 @@ impl FromWorld for WorldId { } } -// SAFETY: Has read-only access to shared World metadata +// SAFETY: No world data is accessed. unsafe impl ReadOnlySystemParam for WorldId {} -// SAFETY: A World's ID is immutable and fetching it from the World does not borrow anything +// SAFETY: No world data is accessed. unsafe impl SystemParam for WorldId { type State = (); @@ -61,7 +61,7 @@ unsafe impl SystemParam for WorldId { world: UnsafeWorldCell<'world>, _: Tick, ) -> Self::Item<'world, 'state> { - world.world_metadata().id() + world.id() } } diff --git a/crates/bevy_ecs/src/world/unsafe_world_cell.rs b/crates/bevy_ecs/src/world/unsafe_world_cell.rs index fe14ed8bfc..9cbab05d27 100644 --- a/crates/bevy_ecs/src/world/unsafe_world_cell.rs +++ b/crates/bevy_ecs/src/world/unsafe_world_cell.rs @@ -1,6 +1,6 @@ #![warn(unsafe_op_in_unsafe_fn)] -use super::{Mut, World}; +use super::{Mut, World, WorldId}; use crate::{ archetype::{Archetype, ArchetypeComponentId, Archetypes}, bundle::Bundles, @@ -190,6 +190,14 @@ impl<'w> UnsafeWorldCell<'w> { unsafe { &*self.0 } } + /// Retrieves this world's unique [ID](WorldId). + #[inline] + pub fn id(self) -> WorldId { + // SAFETY: + // - we only access world metadata + unsafe { self.world_metadata() }.id() + } + /// Retrieves this world's [Entities] collection #[inline] pub fn entities(self) -> &'w Entities { From 27e1cf92ad130416aea97255e0793406a85c8f27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois?= Date: Mon, 29 May 2023 17:25:32 +0200 Subject: [PATCH 05/15] shader_prepass example: disable MSAA for maximum compatibility (#8504) # Objective Since #8446, example `shader_prepass` logs the following error on my mac m1: ``` ERROR bevy_render::render_resource::pipeline_cache: failed to process shader: error: Entry point fragment at Fragment is invalid = Argument 1 varying error = Capability MULTISAMPLED_SHADING is not supported ``` The example display the 3d scene but doesn't change with the preps selected Maybe related to this update in naga: https://github.com/gfx-rs/naga/commit/cc3a8ac73773e2223c3d45fbc1b22607026e2ec0 ## Solution - Disable MSAA in the example, and check if it's enabled in the shader --- assets/shaders/show_prepass.wgsl | 5 +++++ examples/shader/shader_prepass.rs | 2 ++ 2 files changed, 7 insertions(+) diff --git a/assets/shaders/show_prepass.wgsl b/assets/shaders/show_prepass.wgsl index 7369cdadae..cef987d884 100644 --- a/assets/shaders/show_prepass.wgsl +++ b/assets/shaders/show_prepass.wgsl @@ -15,9 +15,14 @@ var settings: ShowPrepassSettings; @fragment fn fragment( @builtin(position) frag_coord: vec4, +#ifdef MULTISAMPLED @builtin(sample_index) sample_index: u32, +#endif #import bevy_pbr::mesh_vertex_output ) -> @location(0) vec4 { +#ifndef MULTISAMPLED + let sample_index = 0u; +#endif if settings.show_depth == 1u { let depth = prepass_depth(frag_coord, sample_index); return vec4(depth, depth, depth, 1.0); diff --git a/examples/shader/shader_prepass.rs b/examples/shader/shader_prepass.rs index d71cede1aa..e7f848a042 100644 --- a/examples/shader/shader_prepass.rs +++ b/examples/shader/shader_prepass.rs @@ -28,6 +28,8 @@ fn main() { }) .add_systems(Startup, setup) .add_systems(Update, (rotate, toggle_prepass_view)) + // Disabling MSAA for maximum compatibility. Shader prepass with MSAA needs GPU capability MULTISAMPLED_SHADING + .insert_resource(Msaa::Off) .run(); } From 6b292d426361c549a8cdaab8d75b83cdff1dafa2 Mon Sep 17 00:00:00 2001 From: Gino Valente <49806985+MrGVSV@users.noreply.github.com> Date: Mon, 29 May 2023 08:29:29 -0700 Subject: [PATCH 06/15] bevy_reflect: Allow `#[reflect(default)]` on enum variant fields (#8514) # Objective When using `FromReflect`, fields can be optionally left out if they are marked with `#[reflect(default)]`. This is very handy for working with serialized data as giant structs only need to list a subset of defined fields in order to be constructed.
Example Take the following struct: ```rust #[derive(Reflect, FromReflect)] struct Foo { #[reflect(default)] a: usize, #[reflect(default)] b: usize, #[reflect(default)] c: usize, #[reflect(default)] d: usize, } ``` Since all the fields are default-able, we can successfully call `FromReflect` on deserialized data like: ```rust ( "foo::Foo": ( // Only set `b` and default the rest b: 123 ) ) ```
Unfortunately, this does not work with fields in enum variants. Marking a variant field as `#[reflect(default)]` does nothing when calling `FromReflect`. ## Solution Allow enum variant fields to define a default value using `#[reflect(default)]`. ### `#[reflect(Default)]` One thing that structs and tuple structs can do is use their `Default` implementation when calling `FromReflect`. Adding `#[reflect(Default)]` to the struct or tuple struct both registers `ReflectDefault` and alters the `FromReflect` implementation to use `Default` to generate any missing fields. This works well enough for structs and tuple structs, but for enums it's not as simple. Since the `Default` implementation for an enum only covers a single variant, it's not as intuitive as to what the behavior will be. And (imo) it feels weird that we would be able to specify default values in this way for one variant but not the others. Because of this, I chose to not implement that behavior here. However, I'm open to adding it in if anyone feels otherwise. --- ## Changelog - Allow enum variant fields to define a default value using `#[reflect(default)]` --- .../bevy_reflect_derive/src/enum_utility.rs | 69 +++++++++++++------ .../bevy_reflect_derive/src/lib.rs | 3 +- crates/bevy_reflect/src/lib.rs | 33 +++++++++ 3 files changed, 83 insertions(+), 22 deletions(-) diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/enum_utility.rs b/crates/bevy_reflect/bevy_reflect_derive/src/enum_utility.rs index a1a8fc8eb7..2a09055e20 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/enum_utility.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/enum_utility.rs @@ -1,10 +1,13 @@ -use crate::fq_std::FQDefault; +use crate::derive_data::StructField; +use crate::field_attributes::DefaultBehavior; +use crate::fq_std::{FQDefault, FQOption}; use crate::{ derive_data::{EnumVariantFields, ReflectEnum}, utility::ident_or_index, }; use proc_macro2::Ident; use quote::{quote, ToTokens}; +use syn::Member; /// Contains all data needed to construct all variants within an enum. pub(crate) struct EnumVariantConstructors { @@ -30,7 +33,7 @@ pub(crate) fn get_variant_constructors( let name = ident.to_string(); let variant_constructor = reflect_enum.get_unit(ident); - let fields = match &variant.fields { + let fields: &[StructField] = match &variant.fields { EnumVariantFields::Unit => &[], EnumVariantFields::Named(fields) | EnumVariantFields::Unnamed(fields) => { fields.as_slice() @@ -39,35 +42,59 @@ pub(crate) fn get_variant_constructors( let mut reflect_index: usize = 0; let constructor_fields = fields.iter().enumerate().map(|(declare_index, field)| { let field_ident = ident_or_index(field.data.ident.as_ref(), declare_index); + let field_ty = &field.data.ty; + let field_value = if field.attrs.ignore.is_ignored() { - quote! { #FQDefault::default() } + match &field.attrs.default { + DefaultBehavior::Func(path) => quote! { #path() }, + _ => quote! { #FQDefault::default() } + } } else { - let error_repr = field.data.ident.as_ref().map_or_else( - || format!("at index {reflect_index}"), - |name| format!("`{name}`"), - ); - let unwrapper = if can_panic { - let type_err_message = format!( - "the field {error_repr} should be of type `{}`", - field.data.ty.to_token_stream() - ); - quote!(.expect(#type_err_message)) + let (resolve_error, resolve_missing) = if can_panic { + let field_ref_str = match &field_ident { + Member::Named(ident) => format!("the field `{ident}`"), + Member::Unnamed(index) => format!("the field at index {}", index.index) + }; + let ty = field.data.ty.to_token_stream(); + + let on_error = format!("{field_ref_str} should be of type `{ty}`"); + let on_missing = format!("{field_ref_str} is required but could not be found"); + + (quote!(.expect(#on_error)), quote!(.expect(#on_missing))) } else { - quote!(?) + (quote!(?), quote!(?)) }; + let field_accessor = match &field.data.ident { Some(ident) => { let name = ident.to_string(); - quote!(.field(#name)) + quote!(#ref_value.field(#name)) } - None => quote!(.field_at(#reflect_index)), + None => quote!(#ref_value.field_at(#reflect_index)), }; reflect_index += 1; - let missing_field_err_message = format!("the field {error_repr} was not declared"); - let accessor = quote!(#field_accessor .expect(#missing_field_err_message)); - quote! { - #bevy_reflect_path::FromReflect::from_reflect(#ref_value #accessor) - #unwrapper + + match &field.attrs.default { + DefaultBehavior::Func(path) => quote! { + if let #FQOption::Some(field) = #field_accessor { + <#field_ty as #bevy_reflect_path::FromReflect>::from_reflect(field) + #resolve_error + } else { + #path() + } + }, + DefaultBehavior::Default => quote! { + if let #FQOption::Some(field) = #field_accessor { + <#field_ty as #bevy_reflect_path::FromReflect>::from_reflect(field) + #resolve_error + } else { + #FQDefault::default() + } + }, + DefaultBehavior::Required => quote! { + <#field_ty as #bevy_reflect_path::FromReflect>::from_reflect(#field_accessor #resolve_missing) + #resolve_error + }, } }; quote! { #field_ident : #field_value } diff --git a/crates/bevy_reflect/bevy_reflect_derive/src/lib.rs b/crates/bevy_reflect/bevy_reflect_derive/src/lib.rs index 086441a311..f7f2859d10 100644 --- a/crates/bevy_reflect/bevy_reflect_derive/src/lib.rs +++ b/crates/bevy_reflect/bevy_reflect_derive/src/lib.rs @@ -87,7 +87,8 @@ pub(crate) static REFLECT_VALUE_ATTRIBUTE_NAME: &str = "reflect_value"; /// to improve performance and/or robustness. /// An example of where this is used is in the [`FromReflect`] derive macro, /// where adding this attribute will cause the `FromReflect` implementation to create -/// a base value using its [`Default`] implementation avoiding issues with ignored fields. +/// a base value using its [`Default`] implementation avoiding issues with ignored fields +/// (for structs and tuple structs only). /// /// ## `#[reflect_value]` /// diff --git a/crates/bevy_reflect/src/lib.rs b/crates/bevy_reflect/src/lib.rs index 4e03f6c12e..40a20a40fe 100644 --- a/crates/bevy_reflect/src/lib.rs +++ b/crates/bevy_reflect/src/lib.rs @@ -751,6 +751,39 @@ mod tests { assert_eq!(Some(expected), my_struct); } + #[test] + fn from_reflect_should_use_default_variant_field_attributes() { + #[derive(Reflect, FromReflect, Eq, PartialEq, Debug)] + enum MyEnum { + Foo(#[reflect(default)] String), + Bar { + #[reflect(default = "get_baz_default")] + #[reflect(ignore)] + baz: usize, + }, + } + + fn get_baz_default() -> usize { + 123 + } + + let expected = MyEnum::Foo(String::default()); + + let dyn_enum = DynamicEnum::new("Foo", DynamicTuple::default()); + let my_enum = ::from_reflect(&dyn_enum); + + assert_eq!(Some(expected), my_enum); + + let expected = MyEnum::Bar { + baz: get_baz_default(), + }; + + let dyn_enum = DynamicEnum::new("Bar", DynamicStruct::default()); + let my_enum = ::from_reflect(&dyn_enum); + + assert_eq!(Some(expected), my_enum); + } + #[test] fn from_reflect_should_use_default_container_attribute() { #[derive(Reflect, FromReflect, Eq, PartialEq, Debug)] From 1b6de76bfb0c1b1929e105f039fd1e69f26f95e2 Mon Sep 17 00:00:00 2001 From: Martin Lysell Date: Mon, 29 May 2023 17:32:11 +0200 Subject: [PATCH 07/15] Disable wasm / webgpu building of wireframe example (#8678) # Objective Remove the wireframe example on the WebGPU examples page as it does not render properly. When run in a browser it will render to all white cube due PolygonMode::LINE not being supported in WebGPU. Relevant docs: https://wgpu.rs/doc/wgpu/struct.Features.html#associatedconstant.POLYGON_MODE_LINE When Rendered with WebGPU: image ## Solution Disable this example when building for WebGPU / wasm. --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 784589e433..7081754991 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -716,7 +716,7 @@ path = "examples/3d/wireframe.rs" name = "Wireframe" description = "Showcases wireframe rendering" category = "3D Rendering" -wasm = true +wasm = false [[example]] name = "no_prepass" From c8deedb0e19f0a7a07c752b26265e6b49d9d1e8b Mon Sep 17 00:00:00 2001 From: JMS55 <47158642+JMS55@users.noreply.github.com> Date: Mon, 29 May 2023 11:36:21 -0400 Subject: [PATCH 08/15] Change default tonemapping method (#8685) Change the default tonemapping method from ReinhardLuminance to TonyMcMapface, which generally looks nicer and works out of the box with bloom. --- ## Changelog - TonyMcMapface is now the default tonemapper, instead of ReinhardLuminance. ## Migration Guide - The default tonemapper has been changed from ReinhardLuminance to TonyMcMapface. Explicitly set ReinhardLuminance on your cameras to get back the previous look. --- crates/bevy_core_pipeline/src/tonemapping/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/bevy_core_pipeline/src/tonemapping/mod.rs b/crates/bevy_core_pipeline/src/tonemapping/mod.rs index 1f080aacab..66f484e864 100644 --- a/crates/bevy_core_pipeline/src/tonemapping/mod.rs +++ b/crates/bevy_core_pipeline/src/tonemapping/mod.rs @@ -134,9 +134,7 @@ pub enum Tonemapping { /// Suffers from lots hue shifting, brights don't desaturate naturally. /// Bright primaries and secondaries don't desaturate at all. Reinhard, - /// Current bevy default. Likely to change in the future. /// Suffers from hue shifting. Brights don't desaturate much at all across the spectrum. - #[default] ReinhardLuminance, /// Same base implementation that Godot 4.0 uses for Tonemap ACES. /// @@ -156,6 +154,7 @@ pub enum Tonemapping { /// Designed as a compromise if you want e.g. decent skin tones in low light, but can't afford to re-do your /// VFX to look good without hue shifting. SomewhatBoringDisplayTransform, + /// Current Bevy default. /// By Tomasz Stachowiak /// /// Very neutral. Subtle but intentional hue shifting. Brights desaturate across the spectrum. @@ -167,6 +166,7 @@ pub enum Tonemapping { /// Color hues are preserved during compression, except for a deliberate [Bezold–Brücke shift](https://en.wikipedia.org/wiki/Bezold%E2%80%93Br%C3%BCcke_shift). /// To avoid posterization, selective desaturation is employed, with care to avoid the [Abney effect](https://en.wikipedia.org/wiki/Abney_effect). /// NOTE: Requires the `tonemapping_luts` cargo feature. + #[default] TonyMcMapface, /// Default Filmic Display Transform from blender. /// Somewhat neutral. Suffers from hue shifting. Brights desaturate across the spectrum. @@ -328,7 +328,7 @@ pub fn get_lut_bindings<'a>( bindings: [u32; 2], ) -> [BindGroupEntry<'a>; 2] { let image = match tonemapping { - //AgX lut texture used when tonemapping doesn't need a texture since it's very small (32x32x32) + // AgX lut texture used when tonemapping doesn't need a texture since it's very small (32x32x32) Tonemapping::None | Tonemapping::Reinhard | Tonemapping::ReinhardLuminance From 292e069bb5e109646552c270088aad68f43b27b7 Mon Sep 17 00:00:00 2001 From: Marco Buono Date: Tue, 30 May 2023 11:21:53 -0300 Subject: [PATCH 09/15] Apply codebase changes in preparation for `StandardMaterial` transmission (#8704) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Objective - Make #8015 easier to review; ## Solution - This commit contains changes not directly related to transmission required by #8015, in easier-to-review, one-change-per-commit form. --- ## Changelog ### Fixed - Clear motion vector prepass using `0.0` instead of `1.0`, to avoid TAA artifacts on transparent objects against the background; ### Added - The `E` mathematical constant is now available for use in shaders, exposed under `bevy_pbr::utils`; - A new `TAA` shader def is now available, for conditionally enabling shader logic via `#ifdef` when TAA is enabled; (e.g. for jittering texture samples) - A new `FallbackImageZero` resource is introduced, for when a fallback image filled with zeroes is required; - A new `RenderPhase::render_range()` method is introduced, for render phases that need to render their items in multiple parceled out “steps”; ### Changed - The `MainTargetTextures` struct now holds both `Texture` and `TextureViews` for the main textures; - The fog shader functions under `bevy_pbr::fog` now take the a `Fog` structure as their first argument, instead of relying on the global `fog` uniform; - The main textures can now be used as copy sources; ## Migration Guide - `ViewTarget::main_texture()` and `ViewTarget::main_texture_other()` now return `&Texture` instead of `&TextureView`. If you were relying on these methods, replace your usage with `ViewTarget::main_texture_view()`and `ViewTarget::main_texture_other_view()`, respectively; - `ViewTarget::sampled_main_texture()` now returns `Option<&Texture>` instead of a `Option<&TextureView>`. If you were relying on this method, replace your usage with `ViewTarget::sampled_main_texture_view()`; - The `apply_fog()`, `linear_fog()`, `exponential_fog()`, `exponential_squared_fog()` and `atmospheric_fog()` functions now take a configurable `Fog` struct. If you were relying on them, update your usage by adding the global `fog` uniform as their first argument; --- crates/bevy_core_pipeline/src/bloom/mod.rs | 4 +- crates/bevy_core_pipeline/src/prepass/node.rs | 5 +- .../bevy_core_pipeline/src/upscaling/node.rs | 2 +- crates/bevy_pbr/src/material.rs | 7 + crates/bevy_pbr/src/render/fog.wgsl | 35 ++-- crates/bevy_pbr/src/render/mesh.rs | 5 + crates/bevy_pbr/src/render/pbr.wgsl | 2 +- crates/bevy_pbr/src/render/pbr_functions.wgsl | 22 +-- crates/bevy_pbr/src/render/utils.wgsl | 1 + crates/bevy_render/src/render_phase/mod.rs | 23 +++ .../bevy_render/src/texture/fallback_image.rs | 39 ++++- crates/bevy_render/src/texture/mod.rs | 1 + crates/bevy_render/src/view/mod.rs | 152 +++++++++++------- 13 files changed, 207 insertions(+), 91 deletions(-) diff --git a/crates/bevy_core_pipeline/src/bloom/mod.rs b/crates/bevy_core_pipeline/src/bloom/mod.rs index 3c41ca716e..9991da1ee2 100644 --- a/crates/bevy_core_pipeline/src/bloom/mod.rs +++ b/crates/bevy_core_pipeline/src/bloom/mod.rs @@ -177,7 +177,9 @@ impl ViewNode for BloomNode { BindGroupEntry { binding: 0, // Read from main texture directly - resource: BindingResource::TextureView(view_target.main_texture()), + resource: BindingResource::TextureView( + view_target.main_texture_view(), + ), }, BindGroupEntry { binding: 1, diff --git a/crates/bevy_core_pipeline/src/prepass/node.rs b/crates/bevy_core_pipeline/src/prepass/node.rs index a0d299bf88..6d3502212d 100644 --- a/crates/bevy_core_pipeline/src/prepass/node.rs +++ b/crates/bevy_core_pipeline/src/prepass/node.rs @@ -67,9 +67,10 @@ impl ViewNode for PrepassNode { view: &view_motion_vectors_texture.default_view, resolve_target: None, ops: Operations { - // Blue channel doesn't matter, but set to 1.0 for possible faster clear + // Red and Green channels are X and Y components of the motion vectors + // Blue channel doesn't matter, but set to 0.0 for possible faster clear // https://gpuopen.com/performance/#clears - load: LoadOp::Clear(Color::rgb_linear(1.0, 1.0, 1.0).into()), + load: LoadOp::Clear(Color::rgb_linear(0.0, 0.0, 0.0).into()), store: true, }, }, diff --git a/crates/bevy_core_pipeline/src/upscaling/node.rs b/crates/bevy_core_pipeline/src/upscaling/node.rs index ae20f46a02..76ff1d195c 100644 --- a/crates/bevy_core_pipeline/src/upscaling/node.rs +++ b/crates/bevy_core_pipeline/src/upscaling/node.rs @@ -47,7 +47,7 @@ impl ViewNode for UpscalingNode { LoadOp::Clear(Default::default()) }; - let upscaled_texture = target.main_texture(); + let upscaled_texture = target.main_texture_view(); let mut cached_bind_group = self.cached_texture_bind_group.lock().unwrap(); let bind_group = match &mut *cached_bind_group { diff --git a/crates/bevy_pbr/src/material.rs b/crates/bevy_pbr/src/material.rs index e9dbce227e..e5db9bee18 100644 --- a/crates/bevy_pbr/src/material.rs +++ b/crates/bevy_pbr/src/material.rs @@ -7,6 +7,7 @@ use bevy_app::{App, Plugin}; use bevy_asset::{AddAsset, AssetEvent, AssetServer, Assets, Handle}; use bevy_core_pipeline::{ core_3d::{AlphaMask3d, Opaque3d, Transparent3d}, + experimental::taa::TemporalAntiAliasSettings, prepass::NormalPrepass, tonemapping::{DebandDither, Tonemapping}, }; @@ -387,6 +388,7 @@ pub fn queue_material_meshes( Option<&DebandDither>, Option<&EnvironmentMapLight>, Option<&NormalPrepass>, + Option<&TemporalAntiAliasSettings>, &mut RenderPhase, &mut RenderPhase, &mut RenderPhase, @@ -401,6 +403,7 @@ pub fn queue_material_meshes( dither, environment_map, normal_prepass, + taa_settings, mut opaque_phase, mut alpha_mask_phase, mut transparent_phase, @@ -417,6 +420,10 @@ pub fn queue_material_meshes( view_key |= MeshPipelineKey::NORMAL_PREPASS; } + if taa_settings.is_some() { + view_key |= MeshPipelineKey::TAA; + } + let environment_map_loaded = match environment_map { Some(environment_map) => environment_map.is_loaded(&images), None => false, diff --git a/crates/bevy_pbr/src/render/fog.wgsl b/crates/bevy_pbr/src/render/fog.wgsl index 852265eefb..e6c1416d93 100644 --- a/crates/bevy_pbr/src/render/fog.wgsl +++ b/crates/bevy_pbr/src/render/fog.wgsl @@ -6,61 +6,66 @@ // https://iquilezles.org/articles/fog/ (Atmospheric Fog and Scattering) fn scattering_adjusted_fog_color( + fog_params: Fog, scattering: vec3, ) -> vec4 { - if (fog.directional_light_color.a > 0.0) { + if (fog_params.directional_light_color.a > 0.0) { return vec4( - fog.base_color.rgb - + scattering * fog.directional_light_color.rgb * fog.directional_light_color.a, - fog.base_color.a, + fog_params.base_color.rgb + + scattering * fog_params.directional_light_color.rgb * fog_params.directional_light_color.a, + fog_params.base_color.a, ); } else { - return fog.base_color; + return fog_params.base_color; } } fn linear_fog( + fog_params: Fog, input_color: vec4, distance: f32, scattering: vec3, ) -> vec4 { - var fog_color = scattering_adjusted_fog_color(scattering); - let start = fog.be.x; - let end = fog.be.y; + var fog_color = scattering_adjusted_fog_color(fog_params, scattering); + let start = fog_params.be.x; + let end = fog_params.be.y; fog_color.a *= 1.0 - clamp((end - distance) / (end - start), 0.0, 1.0); return vec4(mix(input_color.rgb, fog_color.rgb, fog_color.a), input_color.a); } fn exponential_fog( + fog_params: Fog, input_color: vec4, distance: f32, scattering: vec3, ) -> vec4 { - var fog_color = scattering_adjusted_fog_color(scattering); - let density = fog.be.x; + var fog_color = scattering_adjusted_fog_color(fog_params, scattering); + let density = fog_params.be.x; fog_color.a *= 1.0 - 1.0 / exp(distance * density); return vec4(mix(input_color.rgb, fog_color.rgb, fog_color.a), input_color.a); } fn exponential_squared_fog( + fog_params: Fog, input_color: vec4, distance: f32, scattering: vec3, ) -> vec4 { - var fog_color = scattering_adjusted_fog_color(scattering); - let distance_times_density = distance * fog.be.x; + var fog_color = scattering_adjusted_fog_color(fog_params, scattering); + let distance_times_density = distance * fog_params.be.x; fog_color.a *= 1.0 - 1.0 / exp(distance_times_density * distance_times_density); return vec4(mix(input_color.rgb, fog_color.rgb, fog_color.a), input_color.a); } fn atmospheric_fog( + fog_params: Fog, input_color: vec4, distance: f32, scattering: vec3, ) -> vec4 { - var fog_color = scattering_adjusted_fog_color(scattering); - let extinction_factor = 1.0 - 1.0 / exp(distance * fog.be); - let inscattering_factor = 1.0 - 1.0 / exp(distance * fog.bi); + var fog_color = scattering_adjusted_fog_color(fog_params, scattering); + let extinction_factor = 1.0 - 1.0 / exp(distance * fog_params.be); + let inscattering_factor = 1.0 - 1.0 / exp(distance * fog_params.bi); return vec4( input_color.rgb * (1.0 - extinction_factor * fog_color.a) + fog_color.rgb * inscattering_factor * fog_color.a, diff --git a/crates/bevy_pbr/src/render/mesh.rs b/crates/bevy_pbr/src/render/mesh.rs index 3a3834a813..f94c75b370 100644 --- a/crates/bevy_pbr/src/render/mesh.rs +++ b/crates/bevy_pbr/src/render/mesh.rs @@ -586,6 +586,7 @@ bitflags::bitflags! { // See: https://www.khronos.org/opengl/wiki/Early_Fragment_Test const ENVIRONMENT_MAP = (1 << 7); const DEPTH_CLAMP_ORTHO = (1 << 8); + const TAA = (1 << 9); const BLEND_RESERVED_BITS = Self::BLEND_MASK_BITS << Self::BLEND_SHIFT_BITS; // ← Bitmask reserving bits for the blend state const BLEND_OPAQUE = (0 << Self::BLEND_SHIFT_BITS); // ← Values are just sequential within the mask, and can range from 0 to 3 const BLEND_PREMULTIPLIED_ALPHA = (1 << Self::BLEND_SHIFT_BITS); // @@ -804,6 +805,10 @@ impl SpecializedMeshPipeline for MeshPipeline { shader_defs.push("ENVIRONMENT_MAP".into()); } + if key.contains(MeshPipelineKey::TAA) { + shader_defs.push("TAA".into()); + } + let format = if key.contains(MeshPipelineKey::HDR) { ViewTarget::TEXTURE_FORMAT_HDR } else { diff --git a/crates/bevy_pbr/src/render/pbr.wgsl b/crates/bevy_pbr/src/render/pbr.wgsl index f058a83ff8..7043c4e92a 100644 --- a/crates/bevy_pbr/src/render/pbr.wgsl +++ b/crates/bevy_pbr/src/render/pbr.wgsl @@ -133,7 +133,7 @@ fn fragment(in: FragmentInput) -> @location(0) vec4 { // fog if (fog.mode != FOG_MODE_OFF && (material.flags & STANDARD_MATERIAL_FLAGS_FOG_ENABLED_BIT) != 0u) { - output_color = apply_fog(output_color, in.world_position.xyz, view.world_position.xyz); + output_color = apply_fog(fog, output_color, in.world_position.xyz, view.world_position.xyz); } #ifdef TONEMAP_IN_SHADER diff --git a/crates/bevy_pbr/src/render/pbr_functions.wgsl b/crates/bevy_pbr/src/render/pbr_functions.wgsl index 56e3d17773..ee6ac3a79c 100644 --- a/crates/bevy_pbr/src/render/pbr_functions.wgsl +++ b/crates/bevy_pbr/src/render/pbr_functions.wgsl @@ -275,7 +275,7 @@ fn pbr( #endif // PREPASS_FRAGMENT #ifndef PREPASS_FRAGMENT -fn apply_fog(input_color: vec4, fragment_world_position: vec3, view_world_position: vec3) -> vec4 { +fn apply_fog(fog_params: Fog, input_color: vec4, fragment_world_position: vec3, view_world_position: vec3) -> vec4 { let view_to_world = fragment_world_position.xyz - view_world_position.xyz; // `length()` is used here instead of just `view_to_world.z` since that produces more @@ -285,7 +285,7 @@ fn apply_fog(input_color: vec4, fragment_world_position: vec3, view_wo let distance = length(view_to_world); var scattering = vec3(0.0); - if fog.directional_light_color.a > 0.0 { + if fog_params.directional_light_color.a > 0.0 { let view_to_world_normalized = view_to_world / distance; let n_directional_lights = lights.n_directional_lights; for (var i: u32 = 0u; i < n_directional_lights; i = i + 1u) { @@ -295,19 +295,19 @@ fn apply_fog(input_color: vec4, fragment_world_position: vec3, view_wo dot(view_to_world_normalized, light.direction_to_light), 0.0 ), - fog.directional_light_exponent + fog_params.directional_light_exponent ) * light.color.rgb; } } - if fog.mode == FOG_MODE_LINEAR { - return linear_fog(input_color, distance, scattering); - } else if fog.mode == FOG_MODE_EXPONENTIAL { - return exponential_fog(input_color, distance, scattering); - } else if fog.mode == FOG_MODE_EXPONENTIAL_SQUARED { - return exponential_squared_fog(input_color, distance, scattering); - } else if fog.mode == FOG_MODE_ATMOSPHERIC { - return atmospheric_fog(input_color, distance, scattering); + if fog_params.mode == FOG_MODE_LINEAR { + return linear_fog(fog_params, input_color, distance, scattering); + } else if fog_params.mode == FOG_MODE_EXPONENTIAL { + return exponential_fog(fog_params, input_color, distance, scattering); + } else if fog_params.mode == FOG_MODE_EXPONENTIAL_SQUARED { + return exponential_squared_fog(fog_params, input_color, distance, scattering); + } else if fog_params.mode == FOG_MODE_ATMOSPHERIC { + return atmospheric_fog(fog_params, input_color, distance, scattering); } else { return input_color; } diff --git a/crates/bevy_pbr/src/render/utils.wgsl b/crates/bevy_pbr/src/render/utils.wgsl index 379c87e5e4..cb63273171 100644 --- a/crates/bevy_pbr/src/render/utils.wgsl +++ b/crates/bevy_pbr/src/render/utils.wgsl @@ -1,6 +1,7 @@ #define_import_path bevy_pbr::utils const PI: f32 = 3.141592653589793; +const E: f32 = 2.718281828459045; fn hsv2rgb(hue: f32, saturation: f32, value: f32) -> vec3 { let rgb = clamp( diff --git a/crates/bevy_render/src/render_phase/mod.rs b/crates/bevy_render/src/render_phase/mod.rs index 7c7ad41a5e..64a66e7a53 100644 --- a/crates/bevy_render/src/render_phase/mod.rs +++ b/crates/bevy_render/src/render_phase/mod.rs @@ -89,6 +89,29 @@ impl RenderPhase { draw_function.draw(world, render_pass, view, item); } } + + /// Renders all [`PhaseItem`]s in the provided `range` (based on their index in `self.items`) using their corresponding draw functions. + pub fn render_range<'w>( + &self, + render_pass: &mut TrackedRenderPass<'w>, + world: &'w World, + view: Entity, + range: Range, + ) { + let draw_functions = world.resource::>(); + let mut draw_functions = draw_functions.write(); + draw_functions.prepare(world); + + for item in self + .items + .get(range) + .expect("`Range` provided to `render_range()` is out of bounds") + .iter() + { + let draw_function = draw_functions.get_mut(item.draw_function()).unwrap(); + draw_function.draw(world, render_pass, view, item); + } + } } impl RenderPhase { diff --git a/crates/bevy_render/src/texture/fallback_image.rs b/crates/bevy_render/src/texture/fallback_image.rs index cb3bec1420..dedb266343 100644 --- a/crates/bevy_render/src/texture/fallback_image.rs +++ b/crates/bevy_render/src/texture/fallback_image.rs @@ -17,10 +17,21 @@ use crate::{ /// A [`RenderApp`](crate::RenderApp) resource that contains the default "fallback image", /// which can be used in situations where an image was not explicitly defined. The most common /// use case is [`AsBindGroup`] implementations (such as materials) that support optional textures. -/// [`FallbackImage`] defaults to a 1x1 fully white texture, making blending colors with it a no-op. +/// +/// Defaults to a 1x1 fully opaque white texture, (1.0, 1.0, 1.0, 1.0) which makes multiplying +/// it with other colors a no-op. #[derive(Resource, Deref)] pub struct FallbackImage(GpuImage); +/// A [`RenderApp`](crate::RenderApp) resource that contains a _zero-filled_ "fallback image", +/// which can be used in place of [`FallbackImage`], when a fully transparent or black fallback +/// is required instead of fully opaque white. +/// +/// Defaults to a 1x1 fully transparent black texture, (0.0, 0.0, 0.0, 0.0) which makes adding +/// or alpha-blending it to other colors a no-op. +#[derive(Resource, Deref)] +pub struct FallbackImageZero(GpuImage); + /// A [`RenderApp`](crate::RenderApp) resource that contains a "cubemap fallback image", /// which can be used in situations where an image was not explicitly defined. The most common /// use case is [`AsBindGroup`] implementations (such as materials) that support optional textures. @@ -34,9 +45,10 @@ fn fallback_image_new( format: TextureFormat, dimension: TextureViewDimension, samples: u32, + value: u8, ) -> GpuImage { - // TODO make this configurable - let data = vec![255; format.pixel_size()]; + // TODO make this configurable per channel + let data = vec![value; format.pixel_size()]; let extents = Extent3d { width: 1, @@ -92,6 +104,24 @@ impl FromWorld for FallbackImage { TextureFormat::bevy_default(), TextureViewDimension::D2, 1, + 255, + )) + } +} + +impl FromWorld for FallbackImageZero { + fn from_world(world: &mut bevy_ecs::prelude::World) -> Self { + let render_device = world.resource::(); + let render_queue = world.resource::(); + let default_sampler = world.resource::(); + Self(fallback_image_new( + render_device, + render_queue, + default_sampler, + TextureFormat::bevy_default(), + TextureViewDimension::D2, + 1, + 0, )) } } @@ -108,6 +138,7 @@ impl FromWorld for FallbackImageCubemap { TextureFormat::bevy_default(), TextureViewDimension::Cube, 1, + 255, )) } } @@ -148,6 +179,7 @@ impl<'w> FallbackImagesMsaa<'w> { TextureFormat::bevy_default(), TextureViewDimension::D2, sample_count, + 255, ) }) } @@ -171,6 +203,7 @@ impl<'w> FallbackImagesDepth<'w> { TextureFormat::Depth32Float, TextureViewDimension::D2, sample_count, + 255, ) }) } diff --git a/crates/bevy_render/src/texture/mod.rs b/crates/bevy_render/src/texture/mod.rs index 131ca51988..bba263beaa 100644 --- a/crates/bevy_render/src/texture/mod.rs +++ b/crates/bevy_render/src/texture/mod.rs @@ -120,6 +120,7 @@ impl Plugin for ImagePlugin { render_app .insert_resource(DefaultImageSampler(default_sampler)) .init_resource::() + .init_resource::() .init_resource::() .init_resource::() .init_resource::(); diff --git a/crates/bevy_render/src/view/mod.rs b/crates/bevy_render/src/view/mod.rs index 0ab2dd1ed7..e3c66a5519 100644 --- a/crates/bevy_render/src/view/mod.rs +++ b/crates/bevy_render/src/view/mod.rs @@ -13,7 +13,7 @@ use crate::{ render_phase::ViewRangefinder3d, render_resource::{DynamicUniformBuffer, ShaderType, Texture, TextureView}, renderer::{RenderDevice, RenderQueue}, - texture::{BevyDefault, TextureCache}, + texture::{BevyDefault, CachedTexture, TextureCache}, Render, RenderApp, RenderSet, }; use bevy_app::{App, Plugin}; @@ -202,13 +202,16 @@ pub struct PostProcessWrite<'a> { impl ViewTarget { pub const TEXTURE_FORMAT_HDR: TextureFormat = TextureFormat::Rgba16Float; - /// Retrieve this target's color attachment. This will use [`Self::sampled_main_texture`] and resolve to [`Self::main_texture`] if + /// Retrieve this target's color attachment. This will use [`Self::sampled_main_texture_view`] and resolve to [`Self::main_texture`] if /// the target has sampling enabled. Otherwise it will use [`Self::main_texture`] directly. pub fn get_color_attachment(&self, ops: Operations) -> RenderPassColorAttachment { match &self.main_textures.sampled { - Some(sampled_texture) => RenderPassColorAttachment { - view: sampled_texture, - resolve_target: Some(self.main_texture()), + Some(CachedTexture { + default_view: sampled_texture_view, + .. + }) => RenderPassColorAttachment { + view: sampled_texture_view, + resolve_target: Some(self.main_texture_view()), ops, }, None => self.get_unsampled_color_attachment(ops), @@ -221,18 +224,18 @@ impl ViewTarget { ops: Operations, ) -> RenderPassColorAttachment { RenderPassColorAttachment { - view: self.main_texture(), + view: self.main_texture_view(), resolve_target: None, ops, } } /// The "main" unsampled texture. - pub fn main_texture(&self) -> &TextureView { + pub fn main_texture(&self) -> &Texture { if self.main_texture.load(Ordering::SeqCst) == 0 { - &self.main_textures.a + &self.main_textures.a.texture } else { - &self.main_textures.b + &self.main_textures.b.texture } } @@ -242,17 +245,51 @@ impl ViewTarget { /// /// A use case for this is to be able to prepare a bind group for all main textures /// ahead of time. - pub fn main_texture_other(&self) -> &TextureView { + pub fn main_texture_other(&self) -> &Texture { if self.main_texture.load(Ordering::SeqCst) == 0 { - &self.main_textures.b + &self.main_textures.b.texture } else { - &self.main_textures.a + &self.main_textures.a.texture + } + } + + /// The "main" unsampled texture. + pub fn main_texture_view(&self) -> &TextureView { + if self.main_texture.load(Ordering::SeqCst) == 0 { + &self.main_textures.a.default_view + } else { + &self.main_textures.b.default_view + } + } + + /// The _other_ "main" unsampled texture view. + /// In most cases you should use [`Self::main_texture_view`] instead and never this. + /// The textures will naturally be swapped when [`Self::post_process_write`] is called. + /// + /// A use case for this is to be able to prepare a bind group for all main textures + /// ahead of time. + pub fn main_texture_other_view(&self) -> &TextureView { + if self.main_texture.load(Ordering::SeqCst) == 0 { + &self.main_textures.b.default_view + } else { + &self.main_textures.a.default_view } } /// The "main" sampled texture. - pub fn sampled_main_texture(&self) -> Option<&TextureView> { - self.main_textures.sampled.as_ref() + pub fn sampled_main_texture(&self) -> Option<&Texture> { + self.main_textures + .sampled + .as_ref() + .map(|sampled| &sampled.texture) + } + + /// The "main" sampled texture view. + pub fn sampled_main_texture_view(&self) -> Option<&TextureView> { + self.main_textures + .sampled + .as_ref() + .map(|sampled| &sampled.default_view) } #[inline] @@ -290,13 +327,13 @@ impl ViewTarget { // if the old main texture is a, then the post processing must write from a to b if old_is_a_main_texture == 0 { PostProcessWrite { - source: &self.main_textures.a, - destination: &self.main_textures.b, + source: &self.main_textures.a.default_view, + destination: &self.main_textures.b.default_view, } } else { PostProcessWrite { - source: &self.main_textures.b, - destination: &self.main_textures.a, + source: &self.main_textures.b.default_view, + destination: &self.main_textures.a.default_view, } } } @@ -357,9 +394,9 @@ pub fn prepare_view_uniforms( #[derive(Clone)] struct MainTargetTextures { - a: TextureView, - b: TextureView, - sampled: Option, + a: CachedTexture, + b: CachedTexture, + sampled: Option, /// 0 represents `main_textures.a`, 1 represents `main_textures.b` /// This is shared across view targets with the same render target main_texture: Arc, @@ -405,49 +442,50 @@ fn prepare_view_targets( dimension: TextureDimension::D2, format: main_texture_format, usage: TextureUsages::RENDER_ATTACHMENT - | TextureUsages::TEXTURE_BINDING, + | TextureUsages::TEXTURE_BINDING + | TextureUsages::COPY_SRC, view_formats: match main_texture_format { TextureFormat::Bgra8Unorm => &[TextureFormat::Bgra8UnormSrgb], TextureFormat::Rgba8Unorm => &[TextureFormat::Rgba8UnormSrgb], _ => &[], }, }; + let a = texture_cache.get( + &render_device, + TextureDescriptor { + label: Some("main_texture_a"), + ..descriptor + }, + ); + let b = texture_cache.get( + &render_device, + TextureDescriptor { + label: Some("main_texture_b"), + ..descriptor + }, + ); + let sampled = if msaa.samples() > 1 { + let sampled = texture_cache.get( + &render_device, + TextureDescriptor { + label: Some("main_texture_sampled"), + size, + mip_level_count: 1, + sample_count: msaa.samples(), + dimension: TextureDimension::D2, + format: main_texture_format, + usage: TextureUsages::RENDER_ATTACHMENT, + view_formats: descriptor.view_formats, + }, + ); + Some(sampled) + } else { + None + }; MainTargetTextures { - a: texture_cache - .get( - &render_device, - TextureDescriptor { - label: Some("main_texture_a"), - ..descriptor - }, - ) - .default_view, - b: texture_cache - .get( - &render_device, - TextureDescriptor { - label: Some("main_texture_b"), - ..descriptor - }, - ) - .default_view, - sampled: (msaa.samples() > 1).then(|| { - texture_cache - .get( - &render_device, - TextureDescriptor { - label: Some("main_texture_sampled"), - size, - mip_level_count: 1, - sample_count: msaa.samples(), - dimension: TextureDimension::D2, - format: main_texture_format, - usage: TextureUsages::RENDER_ATTACHMENT, - view_formats: descriptor.view_formats, - }, - ) - .default_view - }), + a, + b, + sampled, main_texture: Arc::new(AtomicUsize::new(0)), } }); From ca81d3e4359f158bcf2e20e77d82d7ab52daafc5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9l=C3=A8ne=20Amanita?= <134181069+Selene-Amanita@users.noreply.github.com> Date: Tue, 30 May 2023 15:41:14 +0100 Subject: [PATCH 10/15] Document query errors (#8692) # Objective Add documentation to `Query` and `QueryState` errors in bevy_ecs (`QuerySingleError`, `QueryEntityError`, `QueryComponentError`) ## Solution - Change display message for `QueryEntityError::QueryDoesNotMatch`: this error can also happen when the entity has a component which is filtered out (with `Without`) - Fix wrong reference in the documentation of `Query::get_component` and `Query::get_component_mut` from `QueryEntityError` to `QueryComponentError` - Complete the documentation of the three error enum variants. - Add examples for `QueryComponentError::MissingReadAccess` and `QueryComponentError::MissingWriteAccess` - Add reference to `QueryState` in `QueryEntityError`'s documentation. --- ## Migration Guide Expect `QueryEntityError::QueryDoesNotMatch`'s display message to change? Not sure that counts. --------- Co-authored-by: harudagondi --- crates/bevy_ecs/src/query/state.rs | 17 ++++++-- crates/bevy_ecs/src/system/query.rs | 63 +++++++++++++++++++++++++++-- 2 files changed, 73 insertions(+), 7 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 49b4d72024..b5ccf8263e 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -1162,12 +1162,19 @@ impl QueryState { } } -/// An error that occurs when retrieving a specific [`Entity`]'s query result. +/// An error that occurs when retrieving a specific [`Entity`]'s query result from [`Query`](crate::system::Query) or [`QueryState`]. // TODO: return the type_name as part of this error #[derive(Debug, PartialEq, Eq, Clone, Copy)] pub enum QueryEntityError { + /// The given [`Entity`]'s components do not match the query. + /// + /// Either it does not have a requested component, or it has a component which the query filters out. QueryDoesNotMatch(Entity), + /// The given [`Entity`] does not exist. NoSuchEntity(Entity), + /// The [`Entity`] was requested mutably more than once. + /// + /// See [`QueryState::get_many_mut`] for an example. AliasedMutability(Entity), } @@ -1177,7 +1184,7 @@ impl fmt::Display for QueryEntityError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { QueryEntityError::QueryDoesNotMatch(_) => { - write!(f, "The given entity does not have the requested component.") + write!(f, "The given entity's components do not match the query.") } QueryEntityError::NoSuchEntity(_) => write!(f, "The requested entity does not exist."), QueryEntityError::AliasedMutability(_) => { @@ -1296,11 +1303,13 @@ mod tests { } } -/// An error that occurs when evaluating a [`QueryState`] as a single expected resulted via -/// [`QueryState::single`] or [`QueryState::single_mut`]. +/// An error that occurs when evaluating a [`Query`](crate::system::Query) or [`QueryState`] as a single expected result via +/// [`get_single`](crate::system::Query::get_single) or [`get_single_mut`](crate::system::Query::get_single_mut). #[derive(Debug)] pub enum QuerySingleError { + /// No entity fits the query. NoEntities(&'static str), + /// Multiple entities fit the query. MultipleEntities(&'static str), } diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 4316389ade..1e18705dce 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -1054,7 +1054,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// Returns a mutable reference to the component `T` of the given entity. /// - /// In case of a nonexisting entity or mismatched component, a [`QueryEntityError`] is returned instead. + /// In case of a nonexisting entity or mismatched component, a [`QueryComponentError`] is returned instead. /// /// # Example /// @@ -1090,7 +1090,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// Returns a mutable reference to the component `T` of the given entity. /// - /// In case of a nonexisting entity or mismatched component, a [`QueryEntityError`] is returned instead. + /// In case of a nonexisting entity or mismatched component, a [`QueryComponentError`] is returned instead. /// /// # Safety /// @@ -1357,12 +1357,69 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> IntoIterator for &'w mut Quer } } -/// An error that occurs when retrieving a specific [`Entity`]'s component from a [`Query`] +/// An error that occurs when retrieving a specific [`Entity`]'s component from a [`Query`]. #[derive(Debug, PartialEq, Eq)] pub enum QueryComponentError { + /// The [`Query`] does not have read access to the requested component. + /// + /// This error occurs when the requested component is not included in the original query. + /// + /// # Example + /// + /// ``` + /// # use bevy_ecs::{prelude::*, system::QueryComponentError}; + /// # + /// # #[derive(Component)] + /// # struct OtherComponent; + /// # + /// # #[derive(Component, PartialEq, Debug)] + /// # struct RequestedComponent; + /// # + /// # #[derive(Resource)] + /// # struct SpecificEntity { + /// # entity: Entity, + /// # } + /// # + /// fn get_missing_read_access_error(query: Query<&OtherComponent>, res: Res) { + /// assert_eq!( + /// query.get_component::(res.entity), + /// Err(QueryComponentError::MissingReadAccess), + /// ); + /// println!("query doesn't have read access to RequestedComponent because it does not appear in Query<&OtherComponent>"); + /// } + /// # bevy_ecs::system::assert_is_system(get_missing_read_access_error); + /// ``` MissingReadAccess, + /// The [`Query`] does not have write access to the requested component. + /// + /// This error occurs when the requested component is not included in the original query, or the mutability of the requested component is mismatched with the original query. + /// + /// # Example + /// + /// ``` + /// # use bevy_ecs::{prelude::*, system::QueryComponentError}; + /// # + /// # #[derive(Component, PartialEq, Debug)] + /// # struct RequestedComponent; + /// # + /// # #[derive(Resource)] + /// # struct SpecificEntity { + /// # entity: Entity, + /// # } + /// # + /// fn get_missing_write_access_error(mut query: Query<&RequestedComponent>, res: Res) { + /// assert_eq!( + /// query.get_component::(res.entity), + /// Err(QueryComponentError::MissingWriteAccess), + /// ); + /// println!("query doesn't have write access to RequestedComponent because it doesn't have &mut in Query<&RequestedComponent>"); + /// } + /// # bevy_ecs::system::assert_is_system(get_missing_write_access_error); + /// ``` MissingWriteAccess, + /// The given [`Entity`] does not have the requested component. MissingComponent, + /// The requested [`Entity`] does not exist. NoSuchEntity, } From c8167c1276dd448906d8ef596b22890b063fd676 Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Wed, 31 May 2023 16:57:37 +0200 Subject: [PATCH 11/15] Add `CubicCurve::segment_count` + `iter_samples` adjustment (#8711) ## Objective - Provide a way to use `CubicCurve` non-iter methods - Accept a `FnMut` over a `fn` pointer on `iter_samples` - Improve `build_*_cubic_100_points` benchmark by -45% (this means they are twice as fast) ### Solution Previously, the only way to iterate over an evenly spaced set of points on a `CubicCurve` was to use one of the `iter_*` methods. The return value of those methods were bound by `&self` lifetime, making them unusable in certain contexts. Furthermore, other `CubicCurve` methods (`position`, `velocity`, `acceleration`) required normalizing `t` over the `CubicCurve`'s internal segment count. There were no way to access this segment count, making those methods pretty much unusable. The newly added `segment_count` allows accessing the segment count. `iter_samples` used to accept a `fn`, a function pointer. This is surprising and contrary to the rust stdlib APIs, which accept `Fn` traits for `Iterator` combinators. `iter_samples` now accepts a `FnMut`. I don't trust a bit the bevy benchmark suit, but according to it, this doubles (-45%) the performance on the `build_pos_cubic_100_points` and `build_accel_cubic_100_points` benchmarks. --- ## Changelog - Added the `CubicCurve::segments` method to access the underlying segments of a cubic curve - Allow closures as `CubicCurve::iter_samples` `sample_function` argument. --- crates/bevy_math/src/cubic_splines.rs | 43 +++++++++++++++++++-------- 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/crates/bevy_math/src/cubic_splines.rs b/crates/bevy_math/src/cubic_splines.rs index bb5a988557..c0ff6cf272 100644 --- a/crates/bevy_math/src/cubic_splines.rs +++ b/crates/bevy_math/src/cubic_splines.rs @@ -472,22 +472,39 @@ impl CubicCurve

{ /// A flexible iterator used to sample curves with arbitrary functions. /// /// This splits the curve into `subdivisions` of evenly spaced `t` values across the - /// length of the curve from start (t = 0) to end (t = 1), returning an iterator that evaluates - /// the curve with the supplied `sample_function` at each `t`. + /// length of the curve from start (t = 0) to end (t = n), where `n = self.segment_count()`, + /// returning an iterator evaluating the curve with the supplied `sample_function` at each `t`. /// - /// Given `subdivisions = 2`, this will split the curve into two lines, or three points, and - /// return an iterator over those three points, one at the start, middle, and end. + /// For `subdivisions = 2`, this will split the curve into two lines, or three points, and + /// return an iterator with 3 items, the three points, one at the start, middle, and end. #[inline] - pub fn iter_samples( - &self, + pub fn iter_samples<'a, 'b: 'a>( + &'b self, subdivisions: usize, - sample_function: fn(&Self, f32) -> P, - ) -> impl Iterator + '_ { - (0..=subdivisions).map(move |i| { - let segments = self.segments.len() as f32; - let t = i as f32 / subdivisions as f32 * segments; - sample_function(self, t) - }) + mut sample_function: impl FnMut(&Self, f32) -> P + 'a, + ) -> impl Iterator + 'a { + self.iter_uniformly(subdivisions) + .map(move |t| sample_function(self, t)) + } + + /// An iterator that returns values of `t` uniformly spaced over `0..=subdivisions`. + #[inline] + fn iter_uniformly(&self, subdivisions: usize) -> impl Iterator { + let segments = self.segments.len() as f32; + let step = segments / subdivisions as f32; + (0..=subdivisions).map(move |i| i as f32 * step) + } + + /// The list of segments contained in this `CubicCurve`. + /// + /// This spline's global `t` value is equal to how many segments it has. + /// + /// All method accepting `t` on `CubicCurve` depends on the global `t`. + /// When sampling over the entire curve, you should either use one of the + /// `iter_*` methods or account for the segment count using `curve.segments().len()`. + #[inline] + pub fn segments(&self) -> &[CubicSegment

] { + &self.segments } /// Iterate over the curve split into `subdivisions`, sampling the position at each step. From 5472ea4a142793c58dd81dcf83a4dc47ec904c3c Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Wed, 31 May 2023 12:45:46 -0400 Subject: [PATCH 12/15] Improve encapsulation for commands and add docs (#8725) # Objective Several of our built-in `Command` types are too public: - `GetOrSpawn` is public, even though it only makes sense to call it from within `Commands::get_or_spawn`. - `Remove` and `RemoveResource` contain public `PhantomData` marker fields. ## Solution Remove `GetOrSpawn` and use an anonymous command. Make the marker fields private. --- ## Migration Guide The `Command` types `Remove` and `RemoveResource` may no longer be constructed manually. ```rust // Before: commands.add(Remove:: { entity: id, phantom: PhantomData, }); // After: commands.add(Remove::::new(id)); // Before: commands.add(RemoveResource:: { phantom: PhantomData }); // After: commands.add(RemoveResource::::new()); ``` The command type `GetOrSpawn` has been removed. It was not possible to use this type outside of `bevy_ecs`. --- crates/bevy_ecs/src/system/commands/mod.rs | 41 ++++++++++++---------- 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index e5db31f50b..e0c84dbcaa 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -200,7 +200,9 @@ impl<'w, 's> Commands<'w, 's> { /// apps, and only when they have a scheme worked out to share an ID space (which doesn't happen /// by default). pub fn get_or_spawn<'a>(&'a mut self, entity: Entity) -> EntityCommands<'w, 's, 'a> { - self.add(GetOrSpawn { entity }); + self.add(move |world: &mut World| { + world.get_or_spawn(entity); + }); EntityCommands { entity, commands: self, @@ -839,8 +841,10 @@ where } } +/// A [`Command`] that spawns a new entity and adds the components in a [`Bundle`] to it. #[derive(Debug)] pub struct Spawn { + /// The [`Bundle`] of components that will be added to the newly-spawned entity. pub bundle: T, } @@ -853,16 +857,6 @@ where } } -pub struct GetOrSpawn { - entity: Entity, -} - -impl Command for GetOrSpawn { - fn write(self, world: &mut World) { - world.get_or_spawn(self.entity); - } -} - pub struct SpawnBatch where I: IntoIterator, @@ -907,6 +901,7 @@ where } } +/// A [`Command`] that despawns a specific entity. #[derive(Debug)] pub struct Despawn { pub entity: Entity, @@ -918,8 +913,11 @@ impl Command for Despawn { } } +/// A [`Command`] that adds the components in a [`Bundle`] to an entity. pub struct Insert { + /// The entity to which the components will be added. pub entity: Entity, + /// The [`Bundle`] containing the components that will be added to the entity. pub bundle: T, } @@ -936,10 +934,13 @@ where } } +/// A [`Command`] that removes components from an entity. +/// For a [`Bundle`] type `T`, this will remove any components in the bundle. +/// Any components in the bundle that aren't found on the entity will be ignored. #[derive(Debug)] pub struct Remove { pub entity: Entity, - pub phantom: PhantomData, + _marker: PhantomData, } impl Command for Remove @@ -954,17 +955,19 @@ where } impl Remove { - /// Creates a [`Command`] which will remove the specified [`Entity`] when flushed + /// Creates a [`Command`] which will remove the specified [`Entity`] when applied. pub const fn new(entity: Entity) -> Self { Self { entity, - phantom: PhantomData::, + _marker: PhantomData, } } } +/// A [`Command`] that inserts a [`Resource`] into the world using a value +/// created with the [`FromWorld`] trait. pub struct InitResource { - _phantom: PhantomData, + _marker: PhantomData, } impl Command for InitResource { @@ -977,11 +980,12 @@ impl InitResource { /// Creates a [`Command`] which will insert a default created [`Resource`] into the [`World`] pub const fn new() -> Self { Self { - _phantom: PhantomData::, + _marker: PhantomData, } } } +/// A [`Command`] that inserts a [`Resource`] into the world. pub struct InsertResource { pub resource: R, } @@ -992,8 +996,9 @@ impl Command for InsertResource { } } +/// A [`Command`] that removes the [resource](Resource) `R` from the world. pub struct RemoveResource { - pub phantom: PhantomData, + _marker: PhantomData, } impl Command for RemoveResource { @@ -1006,7 +1011,7 @@ impl RemoveResource { /// Creates a [`Command`] which will remove a [`Resource`] from the [`World`] pub const fn new() -> Self { Self { - phantom: PhantomData::, + _marker: PhantomData, } } } From 5b0f21c7735d13a4e57579f48794430a207beded Mon Sep 17 00:00:00 2001 From: VitalyR Date: Thu, 1 Jun 2023 00:48:03 +0800 Subject: [PATCH 13/15] Add winit's `wayland-csd-adwaita` feature to Bevy's `wayland` feature (#8722) # Objective - Fix Wayland window client side decorations issue on Gnome Wayland, fixes #3301. ## Solution - One simple one line solution: Add winit's `wayland-csd-adwaita` feature to Bevy's `wayland` feature. Copied from https://github.com/bevyengine/bevy/issues/3301#issuecomment-1569611257: ### Investigation 1. Gnome forced Wayland apps to implement CSD, whether on their own or using some libraries like Gnome's official solution [libdecor](https://gitlab.freedesktop.org/libdecor/libdecor). Many Linux apps do this with libdecor, like blender, kitty... I think it's not comfortable for Bevy to fix this problem this way. 2. Winit has support for CSD on wayland(https://github.com/rust-windowing/winit/blob/8bb004a1d9ec1b40cbb9831a6dea774d4b6d6d7b/Cargo.toml#L42), but Bevy disabled Winit's default features, thus no winit's `wayland-csd-adwaita` feature. And Bevy's `wayland` feature doesn't include winit's `wayland-csd-adwaita` feature so users can't get window decorations on Wayland even with Bevy's `wayland` feature enabled. 3. Many rust UI toolkit, like iced, doesn't disable winit's `wayland-csd-adwaita` feature. ### Conclusion and one Possible solution Bevy disabled `winit`'s default features in order to decrease package size. But I think it's acceptable to add `winit`'s `wayland-csd-adwaita` feature to Bevy's `wayland` feature gate to fix this issue easily for this only add on crate: sctk-adwaita. --- crates/bevy_winit/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_winit/Cargo.toml b/crates/bevy_winit/Cargo.toml index c1919a8d62..664c7b05d1 100644 --- a/crates/bevy_winit/Cargo.toml +++ b/crates/bevy_winit/Cargo.toml @@ -10,7 +10,7 @@ keywords = ["bevy"] [features] trace = [] -wayland = ["winit/wayland"] +wayland = ["winit/wayland", "winit/wayland-csd-adwaita"] x11 = ["winit/x11"] accesskit_unix = ["accesskit_winit/accesskit_unix"] From 233b26cc179d064310c056999896a5349b5e14be Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Wed, 31 May 2023 12:49:46 -0400 Subject: [PATCH 14/15] Make the `Condition` trait generic (#8721) # Objective The `Condition` trait is only implemented for systems and system functions that take no input. This can make it awkward to write conditions that are intended to be used with system piping. ## Solution Add an `In` generic to the trait. It defaults to `()`. --- ## Changelog - Made the `Condition` trait generic over system inputs. --- crates/bevy_ecs/src/schedule/condition.rs | 64 +++++++++++++++++++---- 1 file changed, 53 insertions(+), 11 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/condition.rs b/crates/bevy_ecs/src/schedule/condition.rs index 52630acc2b..fa631d3834 100644 --- a/crates/bevy_ecs/src/schedule/condition.rs +++ b/crates/bevy_ecs/src/schedule/condition.rs @@ -8,13 +8,55 @@ use crate::system::{CombinatorSystem, Combine, IntoSystem, ReadOnlySystem, Syste use crate::world::unsafe_world_cell::UnsafeWorldCell; use crate::world::World; -pub type BoxedCondition = Box>; +pub type BoxedCondition = Box>; /// A system that determines if one or more scheduled systems should run. /// -/// Implemented for functions and closures that convert into [`System`](crate::system::System) +/// Implemented for functions and closures that convert into [`System`](crate::system::System) /// with [read-only](crate::system::ReadOnlySystemParam) parameters. -pub trait Condition: sealed::Condition { +/// +/// # Examples +/// A condition that returns true every other time it's called. +/// ``` +/// # use bevy_ecs::prelude::*; +/// fn every_other_time() -> impl Condition<()> { +/// IntoSystem::into_system(|mut flag: Local| { +/// *flag = !*flag; +/// *flag +/// }) +/// } +/// +/// # #[derive(Resource)] struct DidRun(bool); +/// # fn my_system(mut did_run: ResMut) { did_run.0 = true; } +/// # let mut schedule = Schedule::new(); +/// schedule.add_systems(my_system.run_if(every_other_time())); +/// # let mut world = World::new(); +/// # world.insert_resource(DidRun(false)); +/// # schedule.run(&mut world); +/// # assert!(world.resource::().0); +/// # world.insert_resource(DidRun(false)); +/// # schedule.run(&mut world); +/// # assert!(!world.resource::().0); +/// ``` +/// +/// A condition that takes a bool as an input and returns it unchanged. +/// +/// ``` +/// # use bevy_ecs::prelude::*; +/// fn identity() -> impl Condition<(), bool> { +/// IntoSystem::into_system(|In(x)| x) +/// } +/// +/// # fn always_true() -> bool { true } +/// # let mut schedule = Schedule::new(); +/// # #[derive(Resource)] struct DidRun(bool); +/// # fn my_system(mut did_run: ResMut) { did_run.0 = true; } +/// schedule.add_systems(my_system.run_if(always_true.pipe(identity()))); +/// # let mut world = World::new(); +/// # world.insert_resource(DidRun(false)); +/// # schedule.run(&mut world); +/// # assert!(world.resource::().0); +pub trait Condition: sealed::Condition { /// Returns a new run condition that only returns `true` /// if both this one and the passed `and_then` return `true`. /// @@ -59,7 +101,7 @@ pub trait Condition: sealed::Condition { /// Note that in this case, it's better to just use the run condition [`resource_exists_and_equals`]. /// /// [`resource_exists_and_equals`]: common_conditions::resource_exists_and_equals - fn and_then>(self, and_then: C) -> AndThen { + fn and_then>(self, and_then: C) -> AndThen { let a = IntoSystem::into_system(self); let b = IntoSystem::into_system(and_then); let name = format!("{} && {}", a.name(), b.name()); @@ -106,7 +148,7 @@ pub trait Condition: sealed::Condition { /// # app.run(&mut world); /// # assert!(world.resource::().0); /// ``` - fn or_else>(self, or_else: C) -> OrElse { + fn or_else>(self, or_else: C) -> OrElse { let a = IntoSystem::into_system(self); let b = IntoSystem::into_system(or_else); let name = format!("{} || {}", a.name(), b.name()); @@ -114,22 +156,22 @@ pub trait Condition: sealed::Condition { } } -impl Condition for F where F: sealed::Condition {} +impl Condition for F where F: sealed::Condition {} mod sealed { use crate::system::{IntoSystem, ReadOnlySystem}; - pub trait Condition: - IntoSystem<(), bool, Marker, System = Self::ReadOnlySystem> + pub trait Condition: + IntoSystem { // This associated type is necessary to let the compiler // know that `Self::System` is `ReadOnlySystem`. - type ReadOnlySystem: ReadOnlySystem; + type ReadOnlySystem: ReadOnlySystem; } - impl Condition for F + impl Condition for F where - F: IntoSystem<(), bool, Marker>, + F: IntoSystem, F::System: ReadOnlySystem, { type ReadOnlySystem = F::System; From 4ce37395da029eb8b58c78c32812fe8329a3a1a4 Mon Sep 17 00:00:00 2001 From: lelo <15314665+hate@users.noreply.github.com> Date: Wed, 31 May 2023 12:52:36 -0400 Subject: [PATCH 15/15] Add or_else combinator to run_conditions.rs (#8714) # Objective - Showcase the use of `or_else()` as requested. Fixes https://github.com/bevyengine/bevy/issues/8702 ## Solution - Add an uninitialized resource `Unused` - Use `or_else()` to evaluate a second run condition - Add documentation explaining how `or_else()` works --- examples/ecs/run_conditions.rs | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/examples/ecs/run_conditions.rs b/examples/ecs/run_conditions.rs index 413982d6e0..d9f4fc6172 100644 --- a/examples/ecs/run_conditions.rs +++ b/examples/ecs/run_conditions.rs @@ -18,15 +18,20 @@ fn main() { // The common_conditions module has a few useful run conditions // for checking resources and states. These are included in the prelude. .run_if(resource_exists::()) - // This is a custom run condition, defined using a system that returns - // a `bool` and which has read-only `SystemParam`s. - // Both run conditions must return `true` in order for the system to run. - // Note that this second run condition will be evaluated even if the first returns `false`. - .run_if(has_user_input), + // `.or_else()` is a run condition combinator that only evaluates the second condition + // if the first condition returns `false`. This behavior is known as "short-circuiting", + // and is how the `||` operator works in Rust (as well as most C-family languages). + // In this case, the `has_user_input` run condition will be evaluated since the `Unused` resource has not been initialized. + .run_if(resource_exists::().or_else( + // This is a custom run condition, defined using a system that returns + // a `bool` and which has read-only `SystemParam`s. + // Both run conditions must return `true` in order for the system to run. + // Note that this second run condition will be evaluated even if the first returns `false`. + has_user_input, + )), print_input_counter // `.and_then()` is a run condition combinator that only evaluates the second condition - // if the first condition returns `true`. This behavior is known as "short-circuiting", - // and is how the `&&` operator works in Rust (as well as most C-family languages). + // if the first condition returns `true`, analogous to the `&&` operator. // In this case, the short-circuiting behavior prevents the second run condition from // panicking if the `InputCounter` resource has not been initialized. .run_if(resource_exists::().and_then( @@ -51,6 +56,9 @@ fn main() { #[derive(Resource, Default)] struct InputCounter(usize); +#[derive(Resource)] +struct Unused; + /// Return true if any of the defined inputs were just pressed. /// This is a custom run condition, it can take any normal system parameters as long as /// they are read only (except for local parameters which can be mutable).