Skip to content
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

Fix transform for IBL environment maps #12091

Merged
merged 6 commits into from
Jul 24, 2024
Merged

Fix transform for IBL environment maps #12091

merged 6 commits into from
Jul 24, 2024

Conversation

jjhembd
Copy link
Contributor

@jjhembd jjhembd commented Jul 22, 2024

Description

This PR flips the X-axis of the transform applied to IBL environment maps.

Prior to this PR, a mirror ball shows the environment map image in its original orientation, rather than in the expected mirrored view.

The environment used in our test Sandcastles is the Kiara 6 Afternoon image from polyhaven.com. Notice how the sun is to the left of the shadowed peak in the distance. A rocky outcrop in the foreground angles from the sun towards the right of the image.

image

Before this PR, a mirror ball shows the same orientation in the reflection. Again, the sun is to the left of the shadowy peak, with the outcrop angling toward the right.

image

After this PR, the orientation of the environment is flipped from left to right, as expected for a mirror ball reflection.
image

This new orientation matches the glTF Sample Viewer.
image

Issue number and link

Addresses part of #12028.

Testing plan

Load this local Sandcastle and view the "Mirror Ball" model with "Environment Map Lighting".

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @jjhembd!

✅ We can confirm we have a CLA on file for you.

@ggetz
Copy link
Contributor

ggetz commented Jul 23, 2024

Thanks @jjhembd, looks good! Can we add a unit test?

@jjhembd
Copy link
Contributor Author

jjhembd commented Jul 23, 2024

@ggetz OK, I added a spec checking the value of the IBL transform.

@ggetz
Copy link
Contributor

ggetz commented Jul 23, 2024

Thanks @jjhembd. The spec seems to be timing out in CI?

@jjhembd
Copy link
Contributor Author

jjhembd commented Jul 23, 2024

The spec seems to be timing out in CI?

@ggetz Yes, and I can't explain why. The spec passes locally, and similar specs (loading the same environment map) run through on CI.

@ggetz
Copy link
Contributor

ggetz commented Jul 23, 2024

@jjhembd My guess would be the KTX2 loading. I would make sure the URL is correct in both environments and that the environment supports the needed features with OctahedralProjectedCubeMap.isSupported.

@jjhembd
Copy link
Contributor Author

jjhembd commented Jul 23, 2024

@ggetz it's the same URL as the other specs. But the other IBL specs had this check:

        if (!scene.highDynamicRangeSupported) {
          return;
        }

This condition fails on the WebGL stub, so the spec is skipped.

I added the same check to the new spec.

@ggetz
Copy link
Contributor

ggetz commented Jul 24, 2024

Great, thanks @jjhembd!

@ggetz ggetz merged commit ec34c9c into main Jul 24, 2024
9 checks passed
@ggetz ggetz deleted the ibl-transform branch July 24, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants