Skip to content

Conversation

@atlv24
Copy link
Contributor

@atlv24 atlv24 commented Jul 28, 2025

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

@atlv24 atlv24 added A-Rendering Drawing game state to the screen M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 28, 2025
@github-actions
Copy link
Contributor

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.

@atlv24 atlv24 added the D-Shaders This code uses GPU shader languages label Jul 28, 2025
Comment on lines +214 to +222
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
Copy link
Contributor Author

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?

Copy link
Contributor

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.

Comment on lines +97 to +98
let world_pos = world_from_view * vec4(view_pos, 1.0);
return world_pos.xyz;
Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Member

@tychedelia tychedelia left a 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.

@atlv24 atlv24 mentioned this pull request Jul 30, 2025
Copy link
Contributor

@superdump superdump left a 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.

@superdump superdump added this pull request to the merge queue Jul 30, 2025
@superdump
Copy link
Contributor

Turns out it wasn't too late for 0.17.

Merged via the queue into bevyengine:main with commit e08c78b Jul 30, 2025
41 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Jul 30, 2025
github-merge-queue bot pushed a commit that referenced this pull request Jul 30, 2025
# 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 ?
tychedelia pushed a commit to tychedelia/bevy that referenced this pull request Jul 31, 2025
# 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
tychedelia pushed a commit to tychedelia/bevy that referenced this pull request Jul 31, 2025
tychedelia pushed a commit to tychedelia/bevy that referenced this pull request Jul 31, 2025
# 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 ?
@atlv24 atlv24 deleted the ad/view-binding-yeet-1 branch August 1, 2025 01:04
github-merge-queue bot pushed a commit that referenced this pull request Sep 26, 2025
# 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

-
mockersf pushed a commit that referenced this pull request Sep 26, 2025
# 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

-
github-merge-queue bot pushed a commit that referenced this pull request Oct 29, 2025
# 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' .
 
---
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Rendering Drawing game state to the screen D-Shaders This code uses GPU shader languages M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants