Add insert and remove reflected component commands to entity commands #8876
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rror for insert_reflected
|
Can I get a review on this first? This PR makes an already large file even larger. It's not the best in term of maintainability. |
nicopap
added
C-Enhancement
|
Merging the linked PR: once that's in can you please rebase @NoahShomette and then @nicopap can review :) |
Small fix for a forgotten documentation comment.
# Objective Fix bevyengine#1018 (Textures on the `Plane` shape appear flipped). This bug have been around for a very long time apparently, I tested it was still there (see test code bellow) and sure enough, this image:  ... is flipped vertically when used as a texture on a plane (in main, 0.10.1 and 0.9):  I'm pretty confused because this bug is so easy to fix, it has been around for so long, it is easy to encounter, and PRs touching this code still didn't fix it: bevyengine#7546 To the point where I'm wondering if it's actually intended. If it is, please explain why and this PR can be changed to "mention that in the doc". ## Solution Fix the UV mapping on the Plane shape Here is how it looks after the PR  ## Test code ```rust use bevy::{ prelude::*, }; fn main () { App::new() .add_plugins(DefaultPlugins) .add_startup_system(setup) .run(); } fn setup( mut commands: Commands, assets: ResMut<AssetServer>, mut meshes: ResMut<Assets<Mesh>>, mut materials: ResMut<Assets<StandardMaterial>>, ) { commands.spawn(Camera3dBundle { transform: Transform::from_xyz(0., 3., 0.).looking_at(Vec3::ZERO, Vec3::NEG_Z), ..default() }); let mesh = meshes.add(Mesh::from(shape::Plane::default())); let texture_image = assets.load("test.png"); let material = materials.add(StandardMaterial { base_color_texture: Some(texture_image), ..default() }); commands.spawn(PbrBundle { mesh, material, ..default() }); } ``` ## Changelog Fix textures on `Plane` shapes being flipped vertically. ## Migration Guide Flip the textures you use on `Plane` shapes.
 # Objective - Add Screen space ambient occlusion (SSAO). SSAO approximates small-scale, local occlusion of _indirect_ diffuse light between objects. SSAO does not apply to direct lighting, such as point or directional lights. - This darkens creases, e.g. on staircases, and gives nice contact shadows where objects meet, giving entities a more "grounded" feel. - Closes bevyengine#3632. ## Solution - Implement the GTAO algorithm. - https://www.activision.com/cdn/research/Practical_Real_Time_Strategies_for_Accurate_Indirect_Occlusion_NEW%20VERSION_COLOR.pdf - https://blog.selfshadow.com/publications/s2016-shading-course/activision/s2016_pbs_activision_occlusion.pdf - Source code heavily based on [Intel's XeGTAO](https://github.com/GameTechDev/XeGTAO/blob/0d177ce06bfa642f64d8af4de1197ad1bcb862d4/Source/Rendering/Shaders/XeGTAO.hlsli). - Add an SSAO bevy example. ## Algorithm Overview * Run a depth and normal prepass * Create downscaled mips of the depth texture (preprocess_depths pass) * GTAO pass - for each pixel, take several random samples from the depth+normal buffers, reconstruct world position, raytrace in screen space to estimate occlusion. Rather then doing completely random samples on a hemisphere, you choose random _slices_ of the hemisphere, and then can analytically compute the full occlusion of that slice. Also compute edges based on depth differences here. * Spatial denoise pass - bilateral blur, using edge detection to not blur over edges. This is the final SSAO result. * Main pass - if SSAO exists, sample the SSAO texture, and set occlusion to be the minimum of ssao/material occlusion. This then feeds into the rest of the PBR shader as normal. --- ## Future Improvements - Maybe remove the low quality preset for now (too noisy) - WebGPU fallback (see below) - Faster depth->world position (see reverted code) - Bent normals - Try interleaved gradient noise or spatiotemporal blue noise - Replace the spatial denoiser with a combined spatial+temporal denoiser - Render at half resolution and use a bilateral upsample - Better multibounce approximation (https://drive.google.com/file/d/1SyagcEVplIm2KkRD3WQYSO9O0Iyi1hfy/view) ## Far-Future Performance Improvements - F16 math (missing naga-wgsl support https://github.com/gfx-rs/naga/issues/1884) - Faster coordinate space conversion for normals - Faster depth mipchain creation (https://github.com/GPUOpen-Effects/FidelityFX-SPD) (wgpu/naga does not currently support subgroup ops) - Deinterleaved SSAO for better cache efficiency (https://developer.nvidia.com/sites/default/files/akamai/gameworks/samples/DeinterleavedTexturing.pdf) ## Other Interesting Papers - Visibility bitmask (https://link.springer.com/article/10.1007/s00371-022-02703-y, https://cdrinmatane.github.io/posts/cgspotlight-slides/) - Screen space diffuse lighting (https://github.com/Patapom/GodComplex/blob/master/Tests/TestHBIL/2018%20Mayaux%20-%20Horizon-Based%20Indirect%20Lighting%20(HBIL).pdf) ## Platform Support * SSAO currently does not work on DirectX12 due to issues with wgpu and naga: * gfx-rs/wgpu#3798 * gfx-rs/naga#2353 * SSAO currently does not work on WebGPU because r16float is not a valid storage texture format https://gpuweb.github.io/gpuweb/wgsl/#storage-texel-formats. We can fix this with a fallback to r32float. --- ## Changelog - Added ScreenSpaceAmbientOcclusionSettings, ScreenSpaceAmbientOcclusionQualityLevel, and ScreenSpaceAmbientOcclusionBundle --------- Co-authored-by: IceSentry <c.giguere42@gmail.com> Co-authored-by: IceSentry <IceSentry@users.noreply.github.com> Co-authored-by: Daniel Chia <danstryder@gmail.com> Co-authored-by: Elabajaba <Elabajaba@users.noreply.github.com> Co-authored-by: Robert Swain <robert.swain@gmail.com> Co-authored-by: robtfm <50659922+robtfm@users.noreply.github.com> Co-authored-by: Brandon Dyer <brandondyer64@gmail.com> Co-authored-by: Edgar Geier <geieredgar@gmail.com> Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com> Co-authored-by: Carter Anderson <mcanders1@gmail.com>
- Cleanup the `reflect.rs` file in `bevy_ecs`, it's very large and can get difficult to navigate - Split the file into 3 modules, re-export the types in the `reflect/mod.rs` to keep a perfectly identical API. - Add **internal** architecture doc explaining how `ReflectComponent` works. Note that this doc is internal only, since `component.rs` is not exposed publicly. To review this change properly, you need to compare it to the previous version of `reflect.rs`. The diff from this PR does not help at all! What you will need to do is compare `reflect.rs` individually with each newly created file. Here is how I did it: - Adding my fork as remote `git remote add nicopap https://github.com/nicopap/bevy.git` - Checkout out the branch `git checkout nicopap/split_ecs_reflect` - Checkout the old `reflect.rs` by running `git checkout HEAD~1 -- crates/bevy_ecs/src/reflect.rs` - Compare the old with the new with `git diff --no-index crates/bevy_ecs/src/reflect.rs crates/bevy_ecs/src/reflect/component.rs` You could also concatenate everything into a single file and compare against it: - `cat crates/bevy_ecs/src/reflect/{component,resource,map_entities,mod}.rs > new_reflect.rs` - `git diff --no-index crates/bevy_ecs/src/reflect.rs new_reflect.rs`
…rror for insert_reflected
|
I'm not sure what I did with the rebasing but I think I messed it all up :(. I really don't know how to fix it but if the fix is to redo the PR let me know please and I'll get that done right away |
|
Yeah 🤔 Might be easier to just redo this TBH. |
|
yeah. Fairly easy to to create a new file a copy/paste the lines from the file. Here is a link to your changes: https://github.com/bevyengine/bevy/blob/743388a4410a57e5f78cb9c0ff0e8c7c3927f441/crates/bevy_ecs/src/reflect.rs#L473C1-L787 |
|
|
- Cleanup the `reflect.rs` file in `bevy_ecs`, it's very large and can get difficult to navigate - Split the file into 3 modules, re-export the types in the `reflect/mod.rs` to keep a perfectly identical API. - Add **internal** architecture doc explaining how `ReflectComponent` works. Note that this doc is internal only, since `component.rs` is not exposed publicly. To review this change properly, you need to compare it to the previous version of `reflect.rs`. The diff from this PR does not help at all! What you will need to do is compare `reflect.rs` individually with each newly created file. Here is how I did it: - Adding my fork as remote `git remote add nicopap https://github.com/nicopap/bevy.git` - Checkout out the branch `git checkout nicopap/split_ecs_reflect` - Checkout the old `reflect.rs` by running `git checkout HEAD~1 -- crates/bevy_ecs/src/reflect.rs` - Compare the old with the new with `git diff --no-index crates/bevy_ecs/src/reflect.rs crates/bevy_ecs/src/reflect/component.rs` You could also concatenate everything into a single file and compare against it: - `cat crates/bevy_ecs/src/reflect/{component,resource,map_entities,mod}.rs > new_reflect.rs` - `git diff --no-index crates/bevy_ecs/src/reflect.rs new_reflect.rs`
…ected_to_entity_commands' into implement_insert_and_remove_reflected_to_entity_commands # Conflicts: # crates/bevy_ecs/src/reflect/entity_commands.rs # crates/bevy_ecs/src/reflect/mod.rs
NoahShomette
deleted the
implement_insert_and_remove_reflected_to_entity_commands
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
EntityCommandsfunctions for inserting and removing reflected components.Solution
EntityCommandsextension trait for reflection related functions in the reflect module of bevy_ecs.insert_reflectedandremove_reflected. Both functions take aBox<dyn Reflect>component as well as aTypeRegistry. The type registry is used to get the reflection data for the component and enable the inserting and removing.EntityCommandsfields pub(crate) to allow access in the reflect module. (Might be worth making these just public to enable user end custom entity commands?)Note: I'm not entirely sure on cloning the TypeRegistry a bunch. An alternative solution could be to take a reference of the TypeRegistry in the command function, get the registration data out of it right when its called, and then save that data into the command struct. Basically instead of saving the whole TypeRegistry just grab the ReflectComponent out of it and save that into the command struct. However that would involve options and woudl move panics to when the command initially gets called rather than when the command is actually applied. Not sure on exactly how much of a command is allowed to happen before its applied or if this would be preferred. It would prevent cloning the TypeRegistry though.
Changelog
Added:
Box<dyn Reflect>component as well as aTypeRegistry. The type registry is used to get the reflection data for the component and enable the inserting and removing.Changed:
EntityCommandsfields pub(crate) to allow access in the reflect module.