Skip to content

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

Open
beicause wants to merge 1 commit intobevyengine:mainfrom
beicause:fix-mesh-view-env-map-light
Open

Store view environment map rotation in LightProbesUniform and remove EnvironmentMapUniform#24095
beicause wants to merge 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

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-Needs-Review Needs reviewer attention (from anyone!) to move forward

Projects

Status: Needs SME Triage

Development

Successfully merging this pull request may close these issues.

view bindgroup layout changes can cause crash

7 participants