Skip to content

Store view environment map rotation in LightProbesUniform and remove EnvironmentMapUniform#24095

Merged
alice-i-cecile merged 1 commit intobevyengine:mainfrom
beicause:fix-mesh-view-env-map-light
May 6, 2026
Merged

Store view environment map rotation in LightProbesUniform and remove EnvironmentMapUniform#24095
alice-i-cecile merged 1 commit intobevyengine:mainfrom
beicause:fix-mesh-view-env-map-light

Conversation

@beicause
Copy link
Copy Markdown
Member

@beicause beicause commented May 3, 2026

Objective

Fixes #24084 (comment)
Also fixes EnvironmentMapLight::rotation if it is attached to a view.

Solution

Store view environment map rotation in LightProbesUniform and remove EnvironmentMapUniform

Testing

The examples works:

cargo r --example pccm --features free_camera,https
cargo r --example light_probe_blending --features free_camera,https
cargo r --example rotate_environment_map --features pbr_multi_layer_material_textures

@beicause beicause marked this pull request as draft May 3, 2026 09:13
@beicause
Copy link
Copy Markdown
Member Author

beicause commented May 3, 2026

I suspect we can remove the uniform by moving rotation to LightProbe. Also, rotate_environment_map doesn't work.

@Zeophlite Zeophlite added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 3, 2026
@github-project-automation github-project-automation Bot moved this to Needs SME Triage in Rendering May 3, 2026
@Zeophlite Zeophlite added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 3, 2026
@beicause beicause changed the title Add mesh view key for EnvironmentLightMap in a camera Store view environment map rotation in LightProbesUniform and remove EnvironmentMapUniform May 3, 2026
@beicause beicause marked this pull request as ready for review May 3, 2026 11:58
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 3, 2026

Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-24095

If it's expected, please add the M-Deliberate-Rendering-Change label.

If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it.

@beicause beicause added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels May 3, 2026
Comment thread crates/bevy_light/src/probe.rs Outdated
@kfc35 kfc35 self-requested a review May 3, 2026 15:06
@kfc35 kfc35 added this to the 0.19 milestone May 3, 2026
Comment thread crates/bevy_pbr/src/light_probe/mod.rs
@kfc35 kfc35 added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label May 3, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 3, 2026

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.

Copy link
Copy Markdown
Contributor

@kfc35 kfc35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve verified that the quat rotation formula looks correct (although a second pair of eyes wouldn’t hurt there 😅) and that the desired examples run and look as they did previously. I’m assuming the performance impact is minimal? Happy to remove a uniform buffer though to get further under the new webgpu limits.

Also +1 to adding a migration guide for the public facing changes!

Comment thread crates/bevy_pbr/src/light_probe/environment_map.wgsl Outdated
@JMS55
Copy link
Copy Markdown
Contributor

JMS55 commented May 4, 2026

EnvironmentMapLight::rotation on a view wasn't working? Could've sworn we fixed that at some point already.

@beicause
Copy link
Copy Markdown
Member Author

beicause commented May 4, 2026

Hmm, I think rotating before negating z is incorrect:

_20260504_162240.webm

Rotating after negating z:

_20260504_162112.webm

Shouldn't the environment map appear backward (left-handed) when viewed in a mirror?

Debug texture:
cubemap_debug.ktx2.zip

@beicause
Copy link
Copy Markdown
Member Author

beicause commented May 4, 2026

Alright, the second behavior is consistent with 0.18.
I just haven't understood why rotating needs to be changed to after negating z in this PR.

Edit: Rotation needs to be inversed. Now it should be correct.

@kfc35 kfc35 requested a review from JMS55 May 4, 2026 22:50
@cart cart closed this May 5, 2026
@github-project-automation github-project-automation Bot moved this from Needs SME Triage to Done in Rendering May 5, 2026
@cart cart reopened this May 5, 2026
@github-project-automation github-project-automation Bot moved this from Done to Needs SME Triage in Rendering May 5, 2026
@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 5, 2026
@alice-i-cecile
Copy link
Copy Markdown
Member

Broken by the force push noted in #24130; you'll need to clean up the Git history per @cart's message.

@beicause beicause force-pushed the fix-mesh-view-env-map-light branch from 5b0ab61 to 9ce24a7 Compare May 5, 2026 02:44
@kfc35 kfc35 added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels May 5, 2026
Copy link
Copy Markdown
Contributor

@kfc35 kfc35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i checked out your branch and i think the rotate environmental map example looks correct, at least when i imagine myself spinning around with a reflective ball

ShaderRef::Path(AssetPath::from_path_buf(path).with_source("embedded"))
}

pub const TONEMAPPING_LUT_TEXTURE_BINDING_INDEX: u32 = 19;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it because changing the mesh view binding but forgetting to changing it was a footgun - it took me a while to find the cause.

@JMS55 JMS55 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 6, 2026
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 6, 2026
Merged via the queue into bevyengine:main with commit eaa8d42 May 6, 2026
42 checks passed
@github-project-automation github-project-automation Bot moved this from Needs SME Triage to Done in Rendering May 6, 2026
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 C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

view bindgroup layout changes can cause crash

7 participants