Fix #109679: Normal map strength is applied incorrectly #109763

Kaspian Jakobsson · 2023-07-05T23:55:27+02:00

Kaspian Jakobsson commented

2023-07-05 23:55:27 +02:00

Previously the normal strength calculations were linearly extrapolating
the supplied normal map respect to different scaling in the xy resp.
z-axis. This new method takes this into account and applies it in the
correct way.

The calculations are identical to the ones presented in #109679 but
translated into glsl.

Please see #109679 for more information and background.

Previously the error was presented in the following way:
(strength = 20 for clearer example)

Where the left side of the normal map is the node group and the right side is the normal map node

And now they are applied in the same way as the node-group suggested in #109679:

I've attached a test file with a packed normal map which can be used to reference previous version of blender and with this fix applied.
normal_map_test_3.3.blend
normal_map_test.blend

I believe this is the correct way of scaling normals based on the previous discussion but I'm open for suggestions.

Previously the normal strength calculations were linearly extrapolating the supplied normal map respect to different scaling in the xy resp. z-axis. This new method takes this into account and applies it in the correct way. The calculations are identical to the ones presented in #109679 but translated into glsl. Please see #109679 for more information and background. --- Previously the error was presented in the following way: (strength = 20 for clearer example) ![Previous.png](/attachments/78228efe-ee1e-467c-a674-28946f636c5b) Where the left side of the normal map is the node group and the right side is the normal map node And now they are applied in the same way as the node-group suggested in #109679: ![Current.png](/attachments/3e34dbaf-f866-4c33-ba3f-fb67f2b4ed2d) I've attached a test file with a packed normal map which can be used to reference previous version of blender and with this fix applied. [normal_map_test_3.3.blend](/attachments/13fad1db-68ec-4306-b0b6-f6cbff6686a1) [normal_map_test.blend](/attachments/fbbc492e-0eb0-47d1-b914-c1cce1e5763d) I believe this is the correct way of scaling normals based on the previous discussion but I'm open for suggestions.

normal_map_test_3.3.blend

22 MiB

Current.png

2.6 MiB

Previous.png

2.2 MiB

normal_map_test.blend

22 MiB

👍 2 ❤️ 2 🎉 3

Kaspian Jakobsson added 1 commit 2023-07-05 23:55:37 +02:00

Fix #109679 : Normal strength is applied incorrectly 8887b519e8

Previously the normal strength calculations were linearly extrapolating
the supplied normal map respect to different scaling in the xy resp.
z-axis. This new method takes this into account and applies it in the
correct way.

The calculations are identical to the ones presented in #109679 but
translated into glsl.

Please see #109679 for more information and background.

Iliya Katushenock added this to the Render & Cycles project 2023-07-05 23:57:49 +02:00

Iliya Katushenock added the

Module

Viewport & EEVEE

label 2023-07-05 23:57:53 +02:00

Jesse Yurkovich commented

2023-07-06 04:19:58 +02:00

Thanks for the PR! Are you able to attempt a fix for Cycles[1] and OSL[2] as well? The .glsl is only used for Eevee.

[1] https://projects.blender.org/blender/blender/src/branch/main/intern/cycles/kernel/svm/tex_coord.h#L266
[2] https://projects.blender.org/blender/blender/src/branch/main/intern/cycles/kernel/osl/shaders/node_normal_map.osl#L6

Thanks for the PR! Are you able to attempt a fix for Cycles[1] and OSL[2] as well? The .glsl is only used for Eevee. [1] https://projects.blender.org/blender/blender/src/branch/main/intern/cycles/kernel/svm/tex_coord.h#L266 [2] https://projects.blender.org/blender/blender/src/branch/main/intern/cycles/kernel/osl/shaders/node_normal_map.osl#L6

Clément Foucault commented

2023-07-06 12:30:35 +02:00

This looks good to me. Although I am not sure what we are our reference in this case. What standard are we trying to match?

I would also see that fixed for Cycles in the same PR to avoid introducing discrepancies between renderers.
@brecht maybe you have an opinion on this subject since it break compatibility of a core functionality.

Also note that formatting is incorrect. Please run make format in the blender-git directory to fix this.

This looks good to me. Although I am not sure what we are our reference in this case. What standard are we trying to match? I would also see that fixed for Cycles in the same PR to avoid introducing discrepancies between renderers. @brecht maybe you have an opinion on this subject since it break compatibility of a core functionality. Also note that formatting is incorrect. Please run `make format` in the `blender-git` directory to fix this.

Kaspian Jakobsson commented

2023-07-06 13:10:18 +02:00

Thank you for the comments and reviews.
I'll update the PR with Cycles and OSL support, and fix the formatting issues when I get home from work.

Also - I'll update this comment with a bit more background regarding the material I referenced, as to further make sure the change is actually sound :-)!

BR,
Kaspian

Thank you for the comments and reviews. I'll update the PR with Cycles and OSL support, and fix the formatting issues when I get home from work. Also - I'll update this comment with a bit more background regarding the material I referenced, as to further make sure the change is actually sound :-)! BR, Kaspian

Brecht Van Lommel commented

2023-07-06 13:55:49 +02:00

This seems to assume the normal being mixed is in tangent space, but I think it's in world space here? So it would work for a plane pointing along the Z axis, but not other shapes.

👍 2

Sebastian Parborg commented

2023-07-06 14:44:37 +02:00

Also - I'll update this comment with a bit more background regarding the material I referenced, as to further make sure the change is actually sound :-)!

If the logic is deemed to be sound, then be sure to include some explanation comments in the code as well! ;)

> Also - I'll update this comment with a bit more background regarding the material I referenced, as to further make sure the change is actually sound :-)! If the logic is deemed to be sound, then be sure to include some explanation comments in the code as well! ;)

Alex-R-Heskett commented

2023-07-07 07:26:25 +02:00

First-time contributor

This seems to assume the normal being mixed is in tangent space, but I think it's in world space here? So it would work for a plane pointing along the Z axis, but not other shapes.

I noticed the same thing. My original setup is in tangent space, this code will need to be too.

> This seems to assume the normal being mixed is in tangent space, but I think it's in world space here? So it would work for a plane pointing along the Z axis, but not other shapes. I noticed the same thing. My original setup is in tangent space, this code will need to be too.

Kaspian Jakobsson added 4 commits 2023-07-07 22:18:38 +02:00

Fix normals strength from worldspace to tangent space 65ce55fd29

Previously the normal strength was modified in world space instead of
tangent space. The node returns directly after, but keeps the old
method in order to not break ex. object space normals strength

Add Cycles and OSL support for tangent normal map fix 5311419496

This applies the same fix as for Eevee, i.e. the scaled normals
are applied before transforming from tangent space to world space.
Previous behavior is retained for all normals which aren't in tangent
space.

Formatting and commenting code 995676ae33

Formatting

buildbot/vexp-code-patch-coordinator Build done.

Details

61bbfd883f

Kaspian Jakobsson commented

2023-07-07 22:33:54 +02:00

@brecht @Alex-R-Heskett you're both entirely right, it looked correct when doing it in world space as well but I think that was a happy coincidence rather than it being correct! :-)
My latest commit corrects this by simply scaling the x and y components before doing the transform from tangent-space to world-space. I've also added support for Cycles and OSL, formatted and added comments.

Here's a first preview comparing the Eevee edition, it's a bit hard to see on the scaled sphere but it works anyway.

Here's some links for reference:
https://computergraphics.stackexchange.com/questions/5411/correct-way-to-set-normal-strength
https://github.com/TwoTailsGames/Unity-Built-in-Shaders/blob/master/CGIncludes/UnityCG.cginc

With the relevant snippet here

fixed3 UnpackNormalWithScale(fixed4 packednormal, float scale)
{
#ifndef UNITY_NO_DXT5nm
    // Unpack normal as DXT5nm (1, y, 1, x) or BC5 (x, y, 0, 1)
    // Note neutral texture like "bump" is (0, 0, 1, 1) to work with both plain RGB normal and DXT5nm/BC5
    packednormal.x *= packednormal.w;
#endif
    fixed3 normal;
    normal.xy = (packednormal.xy * 2 - 1) * scale;
    normal.z = sqrt(1 - saturate(dot(normal.xy, normal.xy)));
    return normal;
}

I'll add some more comparisons with the Cycles/OSL support soon!

@brecht @Alex-R-Heskett you're both entirely right, it looked correct when doing it in world space as well but I think that was a happy coincidence rather than it being correct! :-) My latest commit corrects this by simply scaling the x and y components before doing the transform from tangent-space to world-space. I've also added support for Cycles and OSL, formatted and added comments. Here's a first preview comparing the Eevee edition, it's a bit hard to see on the scaled sphere but it works anyway. ![prev_current_eevee.png](/attachments/0f833634-b4b9-4495-8f69-4767c3dcc2c4) Here's some links for reference: [https://computergraphics.stackexchange.com/questions/5411/correct-way-to-set-normal-strength](https://computergraphics.stackexchange.com/questions/5411/correct-way-to-set-normal-strength) [https://github.com/TwoTailsGames/Unity-Built-in-Shaders/blob/master/CGIncludes/UnityCG.cginc](https://github.com/TwoTailsGames/Unity-Built-in-Shaders/blob/master/CGIncludes/UnityCG.cginc) With the relevant snippet here ```glsl fixed3 UnpackNormalWithScale(fixed4 packednormal, float scale) { #ifndef UNITY_NO_DXT5nm // Unpack normal as DXT5nm (1, y, 1, x) or BC5 (x, y, 0, 1) // Note neutral texture like "bump" is (0, 0, 1, 1) to work with both plain RGB normal and DXT5nm/BC5 packednormal.x *= packednormal.w; #endif fixed3 normal; normal.xy = (packednormal.xy * 2 - 1) * scale; normal.z = sqrt(1 - saturate(dot(normal.xy, normal.xy))); return normal; } ``` I'll add some more comparisons with the Cycles/OSL support soon!

prev_current_eevee.png

3.2 MiB

👍 1

Kaspian Jakobsson commented

2023-07-08 12:02:26 +02:00

And here is the setup in cycles and OSL!

The previous linear extrapolating behaviour is also kept for object-, world, blender object-and blender world space, in order not to break compability and/or continue a discussion what the desired behavior should be. But I think that might be a discussion for another issue/PR :-)

And here is the setup in cycles and OSL! ![Screenshot 2023-07-08 114341.png](/attachments/a6dc1a4f-c986-4e9a-b2cf-e4fc966fa94c) ![Screenshot 2023-07-08 114259.png](/attachments/f2be4057-b263-4a78-8522-61e25ebd7147) The previous linear extrapolating behaviour is also kept for object-, world, blender object-and blender world space, in order not to break compability and/or continue a discussion what the desired behavior should be. But I think that might be a discussion for another issue/PR :-)

Screenshot 2023-07-08 114259.png

2.7 MiB

Screenshot 2023-07-08 114341.png

2.7 MiB

Brecht Van Lommel requested review from Brecht Van Lommel 2023-07-12 16:43:44 +02:00

Brecht Van Lommel requested review from Clément Foucault 2023-07-12 16:43:44 +02:00

Brecht Van Lommel approved these changes 2023-07-12 16:44:48 +02:00

Brecht Van Lommel left a comment

This looks fine to me now.

It's breaking compatibility, but I think it's acceptable for 4.0, and adding an option or inserting nodes for the old behavior does not seem worth it.

This looks fine to me now. It's breaking compatibility, but I think it's acceptable for 4.0, and adding an option or inserting nodes for the old behavior does not seem worth it.

❤️ 1

Brecht Van Lommel added 1 commit 2023-07-12 17:08:00 +02:00

Merge branch 'main' into HEAD

buildbot/vexp-code-patch-coordinator Build done.

Details

58cbcf1273

Brecht Van Lommel commented

2023-07-12 17:08:47 +02:00

@blender-bot build

Brecht Van Lommel commented

2023-07-12 17:11:14 +02:00

The author name in the git log has some strange special character in it:

Author: <U+0096>kaspian.jakobsson..

When we squash commit this it won't be a problem, but just FYI, you might want to change the config for future commits.

The author name in the `git log` has some strange special character in it: ``` Author: <U+0096>kaspian.jakobsson.. ``` When we squash commit this it won't be a problem, but just FYI, you might want to change the config for future commits.

Brecht Van Lommel added 1 commit 2023-07-12 18:22:32 +02:00

Merge branch 'main' into fix-normals

buildbot/vexp-code-patch-coordinator Build done.

Details

1d41f45e61

Sebastian Parborg commented

2023-07-12 19:10:07 +02:00

Hmm, I think that tangent space is probably what most people are using in their files.
So unless there is a good reason not to, I would think just fixing all of them in one go would be the best as we are already going to break the mode most people are using.

But of course this is up to Clement and Brecht to decide.

Hmm, I think that tangent space is probably what most people are using in their files. So unless there is a good reason not to, I would think just fixing all of them in one go would be the best as we are already going to break the mode most people are using. But of course this is up to Clement and Brecht to decide.

Clément Foucault commented

2023-07-13 10:43:23 +02:00

So unless there is a good reason not to, I would think just fixing all of them in one go would be the best as we are already going to break the mode most people are using.

One reason would be that it is expensive to go into tangent space just for that strength parameter. It is possible, but I don't know if it is worth it.

> So unless there is a good reason not to, I would think just fixing all of them in one go would be the best as we are already going to break the mode most people are using. One reason would be that it is expensive to go into tangent space just for that strength parameter. It is possible, but I don't know if it is worth it.

Clément Foucault approved these changes 2023-07-13 10:44:09 +02:00

Kaspian Jakobsson requested review from Brecht Van Lommel 2023-07-13 13:18:20 +02:00

Kaspian Jakobsson commented

2023-07-13 13:21:22 +02:00

Misclick with the re-requested review @brecht, sorry!

Thank you for all the reviews and comments! I'll look more into whether a implementation that's only dependent on the last mix of normals can be done - as it should be to only scale the normals after the transformation from tangent-space to world-space is applied, right?

I'll submit another PR if that is the case, otherwise for this PR tangent space is probably the best way forward to correct it (for now).

BR,
Kaspian

Misclick with the re-requested review @brecht, sorry! Thank you for all the reviews and comments! I'll look more into whether a implementation that's only dependent on the last mix of normals can be done - as it should be to only scale the normals after the transformation from tangent-space to world-space is applied, right? I'll submit another PR if that is the case, otherwise for this PR tangent space is probably the best way forward to correct it (for now). BR, Kaspian

Brecht Van Lommel approved these changes 2023-07-17 20:05:19 +02:00

Brecht Van Lommel merged commit b767a62f32 into main

2023-07-17 20:06:50 +02:00

Brecht Van Lommel deleted branch fix-normals

2023-07-17 20:06:52 +02:00

Brecht Van Lommel referenced this issue from a commit

2023-07-17 20:06:54 +02:00

Fix #109679: Normal Map node strength interpolation artifacts

Weizhen Huang commented

2023-07-19 17:00:32 +02:00

This patch introduces quite some noise in the benchmark bmw27 scene, because that scene uses a texture with possible negative color.z, causing some normals to be pointing in the wrong hemisphere after this change. Not sure if we should check the sign, but still I would expect the normal to stay unchanged if strength == 0.

This patch introduces quite some noise in the benchmark bmw27 scene, because that scene uses a texture with possible negative `color.z`, causing some normals to be pointing in the wrong hemisphere after this change. Not sure if we should check the sign, but still I would expect the normal to stay unchanged if `strength == 0`.

Roman Markov commented

2023-07-19 18:36:21 +02:00

First-time contributor

This expects the Blue channel of the normal map to be in the 0.5-1 range.

normal_map_test_negative_z.blend

452 KiB

230719_190247_blender_Blender_[CUsersuserDownloadsnormal_map_test.blend].png

767 KiB

Campbell Barton referenced this issue from a commit

2023-07-20 05:06:29 +02:00

Fix #109679: Normal Map node strength interpolation artifacts

William Leeson referenced this issue from a commit

2023-07-21 10:01:54 +02:00

Fix #109679: Normal Map node strength interpolation artifacts

Brecht Van Lommel commented

2023-07-25 15:49:10 +02:00

@kaspian.jakobssongmail.com are you looking into a solution for this? Or shall we solve it?

It wasn't immediately obvious to me how best to do it.

@kaspian.jakobssongmail.com are you looking into a solution for this? Or shall we solve it? It wasn't immediately obvious to me how best to do it.

Kaspian Jakobsson commented

2023-07-25 17:55:19 +02:00

@brecht

@kaspian.jakobssongmail.com are you looking into a solution for this? Or shall we solve it?

It wasn't immediately obvious to me how best to do it.

I'll take a look at it and let you know if I can't fix it in the upcoming days

@brecht > @kaspian.jakobssongmail.com are you looking into a solution for this? Or shall we solve it? > > It wasn't immediately obvious to me how best to do it. I'll take a look at it and let you know if I can't fix it in the upcoming days

Kaspian Jakobsson commented

2023-07-31 07:32:06 +02:00

Hi!
Due to an oversight from my part the z-part wasn't actually scaled but that is now remedied following [1]. The noise introduced in the BMW scene was due to not actually scaling the z-normals whatsoever, but this is corrected with the latest commit.

[1] https://docs.unity3d.com/Packages/com.unity.shadergraph@7.1/manual/Normal-Strength-Node.html

OSL with the patch:

Blender 3.3:

Edit:
Doesn't seem as this PR is following my branch still, unsure how to push to this now since it's merged? Maybe start a new PR?
Here's the relevant commit anyway:
https://projects.blender.org/kaspian.jakobssongmail.com/blender/commit/15615bad476352f840a80a71d2825f0b7952907b

Hi! Due to an oversight from my part the z-part wasn't actually scaled but that is now remedied following [1]. The noise introduced in the BMW scene was due to not actually scaling the z-normals whatsoever, but this is corrected with the latest commit. [1] [https://docs.unity3d.com/Packages/com.unity.shadergraph@7.1/manual/Normal-Strength-Node.html](https://docs.unity3d.com/Packages/com.unity.shadergraph@7.1/manual/Normal-Strength-Node.html) OSL with the patch: ![bmw_osl_patch.png](/attachments/113d2693-1f74-4497-b9f0-fd28f42f200f) Blender 3.3: ![bmw_blender_3.3.png](/attachments/a901fa42-c129-4da9-9f56-8d6fa24ea904) **Edit:** Doesn't seem as this PR is following my branch still, unsure how to push to this now since it's merged? Maybe start a new PR? Here's the relevant commit anyway: [https://projects.blender.org/kaspian.jakobssongmail.com/blender/commit/15615bad476352f840a80a71d2825f0b7952907b](https://projects.blender.org/kaspian.jakobssongmail.com/blender/commit/15615bad476352f840a80a71d2825f0b7952907b)

bmw_blender_3.3.png

574 KiB

bmw_osl_patch.png

582 KiB

Sebastian Parborg commented

2023-07-31 11:17:41 +02:00

@kaspian.jakobssongmail.com you need to create a new PR.
This is because you can't really merge a pr multiple times, or at least you are not supposed to.

@kaspian.jakobssongmail.com you need to create a new PR. This is because you can't really merge a pr multiple times, or at least you are not supposed to.

Roman Markov commented

2023-08-01 13:13:32 +02:00

First-time contributor

The noise in the BMW scene is due to the CarShellNew material using Noise Texture as a normal map which produces RGB values with range 0 to 1 which should be 0.5 to 1 for the blue channel.

If one will search "normal map online" or "generate normal map" the first URL is https://cpetry.github.io/NormalMap-Online/ which generates normal maps with the blue channel with range 0-1 by default.

The noise in the BMW scene is due to the CarShellNew material using Noise Texture as a normal map which produces RGB values with range 0 to 1 which should be 0.5 to 1 for the blue channel. ![230801_133739_blender_Blender_[DDesktopbmw27_cpu.blend].png](/attachments/a10ab63e-e9dd-4d92-87b5-7548111e7b44) ![230801_133753_blender_Blender_[DDesktopbmw27_cpu.blend].png](/attachments/9e43464b-dfad-40d1-bc3f-1090d7a39322) ![230801_133808_blender_Blender_[DDesktopbmw27_cpu.blend].png](/attachments/fc679a8d-c9d4-4e2c-89f2-9b9d57a64fbc) ![230801_133814_blender_Blender_[DDesktopbmw27_cpu.blend].png](/attachments/ebbe8567-4e76-4857-95c6-1ed6af1619b6) ![230801_133829_blender_Blender_[DDesktopbmw27_cpu.blend].png](/attachments/9eb74b8a-c05f-4282-8fe1-e80af041101b) ![230801_133838_blender_Blender_[DDesktopbmw27_cpu.blend].png](/attachments/b441c860-f891-48d3-ac5d-b133b73dd49f) If one will search "normal map online" or "generate normal map" the first URL is https://cpetry.github.io/NormalMap-Online/ which generates normal maps with the blue channel with range 0-1 by default. ![230801_140306_msedge_NormalMap-Online_and_14_more_pages_-_Profile_1_-_M.png](/attachments/052e08f8-03f0-414b-bc69-6085c0c88d9f) ![230801_135509_blender_Blender_[DDesktopbmw27_cpu.blend].png](/attachments/55833300-b029-4e85-bd48-8621d4144d2d) ![230801_140145_blender_Blender_[DDesktopbmw27_cpu.blend].png](/attachments/5edbae81-bfb1-4245-89fd-a8f0e9749cdc) ![230801_140159_blender_Blender_[DDesktopbmw27_cpu.blend].png](/attachments/1b985fc1-99ee-441f-b7de-c7b3c04a81cb)

230801_133739_blender_Blender_[DDesktopbmw27_cpu.blend].png

1.5 MiB

230801_133838_blender_Blender_[DDesktopbmw27_cpu.blend].png

1.4 MiB

230801_133814_blender_Blender_[DDesktopbmw27_cpu.blend].png

1.3 MiB

230801_133808_blender_Blender_[DDesktopbmw27_cpu.blend].png

1.6 MiB

230801_140306_msedge_NormalMap-Online_and_14_more_pages_-_Profile_1_-_M.png

1.8 MiB

230801_133753_blender_Blender_[DDesktopbmw27_cpu.blend].png

1.3 MiB

230801_133829_blender_Blender_[DDesktopbmw27_cpu.blend].png

1.7 MiB

230801_135509_blender_Blender_[DDesktopbmw27_cpu.blend].png

605 KiB

230801_140159_blender_Blender_[DDesktopbmw27_cpu.blend].png

635 KiB

230801_140145_blender_Blender_[DDesktopbmw27_cpu.blend].png

635 KiB

Brecht Van Lommel referenced this issue from a commit

2023-08-01 20:24:18 +02:00

Fix #109679: incorrect handling of negative Z normal maps after recent fix

Brecht Van Lommel commented

2023-08-01 20:33:21 +02:00

Thanks, @kaspian.jakobssongmail.com, I pushed the fix to main. It seems to work well in my tests.

@unwave you can check tomorrow's daily builds to see if solved your examples. It's possible to create normal maps like that where the normals goes below the surface, I'm not sure we'll every be able to render those well but hopefully no worse than before now.

Thanks, @kaspian.jakobssongmail.com, I pushed the fix to main. It seems to work well in my tests. @unwave you can check tomorrow's daily builds to see if solved your examples. It's possible to create normal maps like that where the normals goes below the surface, I'm not sure we'll every be able to render those well but hopefully no worse than before now.

Brecht Van Lommel referenced this issue from a commit

2023-12-05 01:37:12 +01:00

Fix #109679: Normal Map node strength interpolation artifacts

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix #109679: Normal map strength is applied incorrectly #109763