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

[Merged by Bors] - bevy_pbr: Support flipping tangent space normal map y for DirectX normal maps #4433

Closed
wants to merge 1 commit into from

Conversation

superdump
Copy link
Contributor

@superdump superdump commented Apr 6, 2022

Objective

Solution

  • Add a StandardMaterial flip_normal_map_y boolean field
  • Add a STANDARDMATERIAL_FLIP_NORMAL_MAP_Y flag to StandardMaterialFlags and in the PBR shader
  • Flip the y-component of the tangent space normal just after sampling it from the normal map texture

Screenshots

Before

Screenshot 2022-04-06 at 21 04 44

After

Screenshot 2022-04-06 at 21 03 39


Changelog

  • Added: Support for flipping the normal map texture y component for normal maps authored for use with DirectX

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Apr 6, 2022
@superdump superdump added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Apr 6, 2022
@superdump superdump added this to the Bevy 0.7 milestone Apr 6, 2022
@cart
Copy link
Member

cart commented Apr 6, 2022

This is a reasonable fix, but ultimately this feels like a job for an asset preprocessor. A conversation for another day (once we have asset preprocessing).

@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 6, 2022
@superdump
Copy link
Contributor Author

This is a reasonable fix, but ultimately this feels like a job for an asset preprocessor. A conversation for another day (once we have asset preprocessing).

I agree, though maybe this kind of image processing at runtime is undesirable. Imagine wanting to load Bistro as-is from an fbx, and you want to use compressed textures, and compressing each texture takes at least 10s of seconds, and decompressing and compressing is lossy (though maybe there's a way to work around that and do this manipulation in compressed space)... that's pretty infeasible for an online runtime task. Then you would rather just have a .meta file that sets a toggle to flip the y component in the shader. I suppose it could be shader deffed out but then you incur the shader rebind cost when using a mix.

@cart
Copy link
Member

cart commented Apr 6, 2022

Yeah there would likely be cases where "ahead of time" preprocessing wont be viable. But I do think thats probably the direction we should encourage people to go down generally (once thats an option).

@superdump
Copy link
Contributor Author

Yeah there would likely be cases where "ahead of time" preprocessing wont be viable. But I do think thats probably the direction we should encourage people to go down generally (once thats an option).

Yup. I agree. I was looking at image processing stuff in blender/imagemagick first in order to conform the textures first but then decided it was worth doing like this for other reasons too. I agree that we should order preprocessing where possible to avoid runtime costs.

@cart
Copy link
Member

cart commented Apr 7, 2022

bors r+

bors bot pushed a commit that referenced this pull request Apr 7, 2022
…mal maps (#4433)

# Objective

- Normal maps authored for DirectX use a left-handed convention and have their tangent space normal in the texture inverted from what we need. Support this.
- Details here: https://doc.babylonjs.com/divingDeeper/materials/advanced/normalMaps

## Solution

- Add a `StandardMaterial` `flip_normal_map_y` boolean field
- Add a `STANDARDMATERIAL_FLIP_NORMAL_MAP_Y` flag to `StandardMaterialFlags` and in the PBR shader
- Flip the y-component of the tangent space normal just after sampling it from the normal map texture

## Screenshots

### Before

<img width="1392" alt="Screenshot 2022-04-06 at 21 04 44" src="https://user-images.githubusercontent.com/302146/162050314-e7bfaaf6-9ee1-4756-9821-f6f5ff78f508.png">

### After

<img width="1392" alt="Screenshot 2022-04-06 at 21 03 39" src="https://user-images.githubusercontent.com/302146/162050255-36ee0745-1d79-4fd2-9a1c-18085376b643.png">

---

## Changelog

- Added: Support for flipping the normal map texture y component for normal maps authored for use with DirectX
@bors
Copy link
Contributor

bors bot commented Apr 7, 2022

Timed out.

@mockersf
Copy link
Member

mockersf commented Apr 7, 2022

bors r+

bors bot pushed a commit that referenced this pull request Apr 7, 2022
…mal maps (#4433)

# Objective

- Normal maps authored for DirectX use a left-handed convention and have their tangent space normal in the texture inverted from what we need. Support this.
- Details here: https://doc.babylonjs.com/divingDeeper/materials/advanced/normalMaps

## Solution

- Add a `StandardMaterial` `flip_normal_map_y` boolean field
- Add a `STANDARDMATERIAL_FLIP_NORMAL_MAP_Y` flag to `StandardMaterialFlags` and in the PBR shader
- Flip the y-component of the tangent space normal just after sampling it from the normal map texture

## Screenshots

### Before

<img width="1392" alt="Screenshot 2022-04-06 at 21 04 44" src="https://user-images.githubusercontent.com/302146/162050314-e7bfaaf6-9ee1-4756-9821-f6f5ff78f508.png">

### After

<img width="1392" alt="Screenshot 2022-04-06 at 21 03 39" src="https://user-images.githubusercontent.com/302146/162050255-36ee0745-1d79-4fd2-9a1c-18085376b643.png">

---

## Changelog

- Added: Support for flipping the normal map texture y component for normal maps authored for use with DirectX
@bors bors bot changed the title bevy_pbr: Support flipping tangent space normal map y for DirectX normal maps [Merged by Bors] - bevy_pbr: Support flipping tangent space normal map y for DirectX normal maps Apr 7, 2022
@bors bors bot closed this Apr 7, 2022
aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
…mal maps (bevyengine#4433)

# Objective

- Normal maps authored for DirectX use a left-handed convention and have their tangent space normal in the texture inverted from what we need. Support this.
- Details here: https://doc.babylonjs.com/divingDeeper/materials/advanced/normalMaps

## Solution

- Add a `StandardMaterial` `flip_normal_map_y` boolean field
- Add a `STANDARDMATERIAL_FLIP_NORMAL_MAP_Y` flag to `StandardMaterialFlags` and in the PBR shader
- Flip the y-component of the tangent space normal just after sampling it from the normal map texture

## Screenshots

### Before

<img width="1392" alt="Screenshot 2022-04-06 at 21 04 44" src="https://user-images.githubusercontent.com/302146/162050314-e7bfaaf6-9ee1-4756-9821-f6f5ff78f508.png">

### After

<img width="1392" alt="Screenshot 2022-04-06 at 21 03 39" src="https://user-images.githubusercontent.com/302146/162050255-36ee0745-1d79-4fd2-9a1c-18085376b643.png">

---

## Changelog

- Added: Support for flipping the normal map texture y component for normal maps authored for use with DirectX
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…mal maps (bevyengine#4433)

# Objective

- Normal maps authored for DirectX use a left-handed convention and have their tangent space normal in the texture inverted from what we need. Support this.
- Details here: https://doc.babylonjs.com/divingDeeper/materials/advanced/normalMaps

## Solution

- Add a `StandardMaterial` `flip_normal_map_y` boolean field
- Add a `STANDARDMATERIAL_FLIP_NORMAL_MAP_Y` flag to `StandardMaterialFlags` and in the PBR shader
- Flip the y-component of the tangent space normal just after sampling it from the normal map texture

## Screenshots

### Before

<img width="1392" alt="Screenshot 2022-04-06 at 21 04 44" src="https://user-images.githubusercontent.com/302146/162050314-e7bfaaf6-9ee1-4756-9821-f6f5ff78f508.png">

### After

<img width="1392" alt="Screenshot 2022-04-06 at 21 03 39" src="https://user-images.githubusercontent.com/302146/162050255-36ee0745-1d79-4fd2-9a1c-18085376b643.png">

---

## Changelog

- Added: Support for flipping the normal map texture y component for normal maps authored for use with DirectX
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-Usability A targeted quality-of-life change that makes Bevy easier to use 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.

3 participants