-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Move view transformations to bevy_render #20313
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ransformations.wgsl
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
| fn depth_ndc_to_view_z(ndc_depth: f32, clip_from_view: mat4x4<f32>, view_from_clip: mat4x4<f32>) -> f32 { | ||
| #ifdef VIEW_PROJECTION_PERSPECTIVE | ||
| return -perspective_camera_near(clip_from_view) / ndc_depth; | ||
| #else ifdef VIEW_PROJECTION_ORTHOGRAPHIC | ||
| return -(clip_from_view[3][2] - ndc_depth) / clip_from_view[2][2]; | ||
| #else | ||
| let view_pos = view_from_clip * vec4(0.0, 0.0, ndc_depth, 1.0); | ||
| return view_pos.z / view_pos.w; | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: the need for two matrices here smells funny to me. why do we need this? is there a way to write it in terms of one matrix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because of optimisations. Using values from the forward transform (clip from view) to invert it.
| let world_pos = world_from_view * vec4(view_pos, 1.0); | ||
| return world_pos.xyz; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: most of these functions are the same thing with different names. "augment point with w = 1.0, mul, return xyz" or same thing but with w = 0.0 for directions. Do we want to unify them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure it's worth it. These functions are mostly so people don't need to understand exactly what the correct maths should be, I guess. Convenience. But extracting functions for... transform_position() and transform_direction() seems unnecessary to me.
tychedelia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes a lot of sense to me.
I think in the future, we may be able to better play with code generation, such that these functions link to a module that is provided at runtime by the WESL loader and contains the resources, but we can experiment with that later.
superdump
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this but it's probably too late for 0.17.
|
Turns out it wasn't too late for 0.17. |
# Objective - add migration guide for #20313
# Objective - split off bevy_shader from bevy_render to allow for defining shader libraries and materials for scenes without depending on a renderer ## Solution - a bunch of small refactor commits, then bevy_shader! (review commit by commit) ## Testing - please help with this, especially with spirv passthrough and decoupled_naga testing: i only ran 3d_scene at every step to make sure i didnt break anything. EDIT: this has been tested now, decoupled_naga works, and I also fixed a bug that was on main when wesl and decoupled_naga were both present it didnt compile, now it does # Future Work - Split ShaderCacheError out of PipelineCacheError (or just rename) (punted because breaking change) - Consider renaming PipelineCacheId to ShaderCacheId ? (punted because breaking change) - bevy_material - bevy_bsdf (bindingless pbr shader library) - move view.wgsl into bevy_camera after #20313 lands ?
# Objective - having view_transformations in bevy_pbr just because they need the bindings sucks, it hinders code reuse and means we often can't use these functions on custom view bindings. ## Solution - Add binding-less version of view transformations and deprecate view_transformations.wgsl ## Testing - 3d_scene runs # The Plan: this is a "trial run" first step: it includes no migration of the existing code-base, and does no breaking changes, its just a minimal example of what going in this direction will look like. If we like this direction i'll sink more time into this, writing a migration guide and some followup prs: - migrations for the codebase - removal of the deprecated functions (in 0.18, probably) - similar PRs for PBR functions
…0338) # Objective - add migration guide for bevyengine#20313
# Objective - split off bevy_shader from bevy_render to allow for defining shader libraries and materials for scenes without depending on a renderer ## Solution - a bunch of small refactor commits, then bevy_shader! (review commit by commit) ## Testing - please help with this, especially with spirv passthrough and decoupled_naga testing: i only ran 3d_scene at every step to make sure i didnt break anything. EDIT: this has been tested now, decoupled_naga works, and I also fixed a bug that was on main when wesl and decoupled_naga were both present it didnt compile, now it does # Future Work - Split ShaderCacheError out of PipelineCacheError (or just rename) (punted because breaking change) - Consider renaming PipelineCacheId to ShaderCacheId ? (punted because breaking change) - bevy_material - bevy_bsdf (bindingless pbr shader library) - move view.wgsl into bevy_camera after bevyengine#20313 lands ?
# Objective - Fix chrome rendering bug: <img width="1755" height="985" alt="image" src="https://github.com/user-attachments/assets/6293a960-b8fa-460e-9d71-e7f2a9fb2d20" /> ## Solution - Revert part of #20313 - I don't know why this fixes it, but #20960 also ran into it ## Testing -
# Objective - Fix chrome rendering bug: <img width="1755" height="985" alt="image" src="https://github.com/user-attachments/assets/6293a960-b8fa-460e-9d71-e7f2a9fb2d20" /> ## Solution - Revert part of #20313 - I don't know why this fixes it, but #20960 also ran into it ## Testing -
# Objective This PR is to add SSAO support on WebGPU. ## Solution Add r16float detect and fallback to r32float if not supported. FYI the initial ssao PR [here](#7402). ## Testing - Did you test these changes? If so, how? Yes, here is the command to test the ssao example `RUSTFLAGS="--cfg getrandom_backend=\"wasm_js\"" cargo run --example ssao --target wasm32-unknown-unknown --features webgpu` and then http://localhost:1334, make sure it supports [WebGPU](https://caniuse.com/webgpu) - Are there any parts that need more testing? N/A - How can other people (reviewers) test your changes? Is there anything specific they need to know? 1. For the R32Float fallback, only the 1st commit is needed. However, I ran into some strange flickering issue on Chrome (and Chrome canary too), so I added a "detect_r16float_support" flag and a temp fix in the 2nd and 3rd commit, which are intended to be reverted when merging. 2. with the 1st commit, Safari works fine, but Chrome has the flickering, and I fixed it by partially reverting a shader change from #20313, which is very strange. It is probably caused by some shader optimization issue in Chrome, and I have no clue why, as the change is just moving the calculation to a different function. Here is the flickering on Mac Chrome(140.0.7339.133 (Official Build) (arm64)) <img width="353" height="327" alt="chrome_ssao" src="https://github.com/user-attachments/assets/0666673f-508e-4b57-a152-19327ffdb6f3" /> - If relevant, what platforms did you test these changes on, and are there any important ones you can't test? Tested on native(mac) and WebGPU. To test on native, set "detect_r16float_support" to false to force R32Float. I couldn't make the WebGPU ssao example running on Windows 11 Chrome or Firefox, there is some error in 'taa_pipeline' . ---
Objective
Solution
Testing
The Plan:
this is a "trial run" first step: it includes no migration of the existing code-base, and does no breaking changes, its just a minimal example of what going in this direction will look like. If we like this direction i'll sink more time into this, writing a migration guide and some followup prs: