mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
101.23k stars 35.25k forks source link

Seams with normal map and mirrored uvs #18565

Open njarraud opened 4 years ago

njarraud commented 4 years ago
Description of the problem

I guess it may not be just a threejs issue but I ran into a problem when using a normal map with a mesh having mirrored uvs.

With the current code, my understanding is that the RGB value (128, 128, 255) which is supposed to represent a non perturbed normal by convention is read as (0.50196, 0.50196, 1.0) - in linear color space. When converted to [-1, 1] using the function map * 2.0 - 1.0 the vector encoded in the texture becomes (0.00392, 0.00392, 1.0) when it shall be (0.0, 0.0, 1.0). This is the reason why when using a mesh with mirrored uvs and a normal texture, the two normals at a seam have different directions creating the artifact.

It can be easily proven by modifying the perturbNormal2Arb function. Replace vec3 mapN = map * 2.0 - 1.0; by vec3 mapN = map * 2.0 - 1.00392; and the seams disappear. I attached two screenshots made using a fully metallic material and no roughness to make it very obvious:

seams

noseams

Not too sure how this problem is generally solved as this is not my area of expertise.

Three.js version
Browser
OS
Mugen87 commented 4 years ago

I'm not sure but I think you hit here a design limitation. AFAICS, Babylon.js implements similar code in their perturbNormal() method:

https://github.com/BabylonJS/Babylon.js/blob/8bdf14354f66d12d10d61114489e1de13a9c66f8/src/Shaders/ShadersInclude/bumpFragmentFunctions.fx#L12

BTW: It seems the code in normalmap_pars_fragment and NormalMapNode is a bit out of sync. The node shader code still has a workaround that was removed from the core here: #17958

njarraud commented 4 years ago

Blender may also use the same type of calculation as it requires the same hack to make it work. It is maybe more understandable as asset size is maybe less of an issue and 32bit solves it.

blender

That is really weird though because you would think that the neutral vector is kind of the base line. Anyway, I'll continue to use my hack as it does the trick perfectly. Mirroring allows to use much smaller normal textures which means smaller loading time and better GPU performance.

makc commented 4 years ago

so the root of the problem is, the 128 used for 0 where 127.5 should be?

map * 2.0 - 1.00392

btw for me the 1st approximation that gives 0 for 128 in the console is 1.003921568627451: Screen Shot 2020-02-08 at 1 31 41

makc commented 4 years ago

it seems you need something like this to be on the safe side: Screen Shot 2020-02-08 at 1 36 07

donmccurdy commented 4 years ago

Can you share the model?

Some issues like this can be avoided by computing vertex tangents and setting material.vertexTangents = true. You can add tangents with an option in the Blender glTF exporter, or with BufferGeometryUtils.

makc commented 4 years ago

@donmccurdy I actually get 'vTangent' : undeclared identifier shader error trying to do what you say 😊 here is the fiddle reproducing @njarraud issue, if you uncomment lines 32-34 it explodes.

donmccurdy commented 4 years ago

@makc use MeshStandardMaterial and there are no errors:

material = new THREE.MeshStandardMaterial({
  metalness: 1,
  roughness: 0,
  normalMap: normalMap,
  envMap: texture
});

I would've expected it to work for Phong, but not Basic or Lambert materials. We may need to update docs in that case. I'm also surprised you don't need to clone the material before setting .vertexTangents=true, but apparently it works without that.

makc commented 4 years ago

@donmccurdy ok it does not break with MeshStandardMaterial, that is something. but the seam issue is still there even after computing tangents - https://jsfiddle.net/jz64ow0m/

donmccurdy commented 4 years ago

Hm, ok β€” my earlier suggestion is in the right direction, but isn't going to work. On the bright side I think I've learned enough to explain the issue here. From PolyCount β€’ Normal Map Technical Details:

When you look at a tangent-space normal map for a character, you typically see different colors along the UV seams. This is because the UV shells are often oriented at different angles on the mesh, a necessary evil when translating the 3D mesh into 2D textures. The body might be mapped with a vertical shell, and the arm mapped with a horizontal one. This requires the normals in the normal map to be twisted for the different orientations of those UV shells. The UVs are twisted, so the normals must be twisted in order to compensate. The tangent basis helps reorient (twist) the lighting as it comes into the surface's local space, so the lighting will then look uniform across the normal mapped mesh.

To eliminate seams and shading artifacts, the game engine and the normal map baking tool should use the same tangent basis. ... When you export a model from your baking tool, it is best to choose a format like FBX which can store the tangents. This insures the same tangents which were used for baking, will be used for rendering.

In other words, my suggestion of computing tangents using BufferGeometryUtils is no good β€” the normal map needs to be created for those tangents, because the whole thing is relative to a tangent basis.

I tried to reproduce your JSFiddle in Blender, by creating a plane, cutting it into two pieces, then UV unwrapping it with a very obvious seam: the two UV islands are rotated and separated from one another. I then baked the normal map in Blender, and you can see that even though it's a flat plane, the normal map has a different color for each UV island because of the different UV orientation:

Screen Shot 2020-02-11 at 8 26 00 PM

Unfortunately I've hit a bug while trying to export the tangents from that model: the seam is reduced, but still visible, and all reflections are rotated by 90ΒΊ. I think that rotation is due to Blender's different +Z=up convention, and needs to be fixed in the glTF exporter.

In short: normal maps crossing UV seams are hard, and I don't expect any change to perturbNormal2Arb is going to solve it. If you baked your normal map using software based on MikktSpace, which many programs use, then computing tangents with that same algorithm would work. But for an arbitrary normal map, if you don't have tangents and don't know what convention it was baked with, that UV probably seam isn't going away... here's the model from above, without tangents, and with a flat normal map instead of the baked one. I don't expect any tweak to perturbNormal2Arb can fix this seam:

NormalTest_naive.glb.zip

donmccurdy commented 4 years ago

/cc https://github.com/KhronosGroup/glTF-Blender-IO/issues/920

donmccurdy commented 4 years ago

A note about this line:

it is best to choose a format like FBX which can store the tangents...

I'm only aware of two formats that store tangents: glTF and FBX. THREE.FBXLoader doesn't load tangents currently (@looeee I can try to create an example if we want to add this?), so GLTFLoader is the only option for now.

donmccurdy commented 4 years ago

And... here's one much, much simpler option if you want to avoid all that: use an object-space normal map instead of a tangent-space one. https://jsfiddle.net/donmccurdy/4zx1aumq/

screenshot
before Screen Shot 2020-02-11 at 9 31 53 PM
after Screen Shot 2020-02-11 at 9 32 00 PM
makc commented 4 years ago

@donmccurdy I dont know why you need to write the research paper above when topic starter already identified both the problem and the solution :)

In short: normal maps crossing UV seams are hard, and I don't expect any change to perturbNormal2Arb is going to solve it.

No matter if you expect it to solve the problem or not, it does solve it - https://jsfiddle.net/cuotmdfL/

if you want to avoid all that: use an object-space normal map instead of a tangent-space

or do not use normal map or do not use geometry mirroring

it is like saying, just do not run into a bug. it works, butnot really helping it :)

donmccurdy commented 4 years ago
THREE.ShaderChunk.normal_fragment_maps = THREE.ShaderChunk.normal_fragment_maps
  .split("texture2D( normalMap, vUv ).xyz * 2.0 - 1.0;")
  .join("texture2D( normalMap, vUv ).xyz * 2.0 - 1.003921568627451;")

I see your point, it fixes the specific issue reported here, but ... this patch is not a general solution to UV seams in normal maps. Please feel free to try the same patch on the model attached above. 😬

Do you feel it should be merged to normal_fragment_maps.glsl?

makc commented 4 years ago

Do you feel it should be merged

it makes sense if it is established that all the normal maps out there use 128 for zero. if some round down to 127 instead, there again will be a problem, and I did not test enough normal maps on pixel level to be sure.

looeee commented 4 years ago

THREE.FBXLoader doesn't load tangents currently (@looeee I can try to create an example if we want to add this?)

It would be interesting to add this, although so far I haven't seen any requests for this feature. Since I've never used tangents it would take me some time to study how they work so that I could add them to the loader. Sounds like fun, but it's not something I have time for at the moment, unfortunately.

In the meantime, I was able to create examples. Here's sphere and cube meshes in ASCII and binary FBX format with tangents and binormals (it's not possible to export just tangents, so maybe they always need to be included together?).

fbx_sphere_cube_tangents.zip

donmccurdy commented 4 years ago

@looeee ok, no problem! We had a couple models reported in glTF where UV seams and mirroring issues could be fixed with tangents, but I haven't seen a similar report for FBX, and understand if you don't want to spend time on it until/unless that happens. :)

makc commented 4 years ago

@donmccurdy > Please feel free to try the same patch on the model attached above

and btw, it just so happens that the patch in question does, in fact, remove the seam on the model attached above - https://jsfiddle.net/742du8gv/ - under the condition that the normal map you saved there is replaced by single color 128,128,255 normal map, because yours is not entirely uniform. however, even with your normal map left as is the seam is significantly improved :P

donmccurdy commented 4 years ago

Alright, you win. πŸ˜… The "flat" and "baked" normal maps I used are attached below. I'm not sure why the one embedded in the file, the smaller image below, needed to be replaced – it was a cropped flat section from another normal map I found online.

original baked
normal NormalBaked
makc commented 4 years ago

Just to follow up, here are fiddles based on https://github.com/mrdoob/three.js/issues/18565#issuecomment-583661316:

As you see, there are no seams at both 127 and 128 -based maps, but the expression becomes much less elegant.

cptSwing commented 4 years ago

Can you share the model?

Some issues like this can be avoided by computing vertex tangents and setting material.vertexTangents = true. You can add tangents with an option in the Blender glTF exporter, or with BufferGeometryUtils.

Are these calculated according to MikkTSpace? (sorry, slightly offtopic)

Mugen87 commented 4 years ago

At least BufferGeometryUtils.computeTangents() does not implement MikkTSpace.

donmccurdy commented 4 years ago

I'm not aware of any JavaScript MikktSpace implementation. There's no reason it couldn't be done, but it's a fair bit of work. And (in this particular case) it isn't the solution.

cptSwing commented 4 years ago

Thank you fellas. I wasn't sure what the last word was on the tangent/threejs issue and it would have saved me one step in my 3D pipeline.. alas, so be it ;)

Mugen87 commented 4 years ago

I would've expected it to work for Phong, but not Basic or Lambert materials.

With #18614, vertexTangents is only supported in MeshStandardMaterial.

BTW: Is it okay to close this issue? AFAICS, the issue depends on how the model was authored, right?

makc commented 4 years ago

@Mugen87 TL;DR the issue was about neither 127 nor 128 sitting exactly in a middle between 0 and 255 - and then in certain situations that 0.5/255 difference is enough to create a seam.

the issue depends on how the model was authored, right?

that is correct, if not fixed you cant use the models auhored in this way - so no more

much smaller normal textures which means smaller loading time and better GPU performance

for you :)

Mugen87 commented 4 years ago

if not fixed you cant use the models auhored in this way - so no more

How does this fix look like? Is it the code from https://github.com/mrdoob/three.js/issues/18565#issuecomment-585042708?

makc commented 4 years ago

Mugen, there were a couple of hacks discussed in the thread, but no PR so far. The simplest hack offered by @njarraud shifts normal map zero level to 128, thus causing to 0 and 255 to map incorrectly as a price. I posted fiddles for both this hack and 127-based hack I think. Then the more convoluted hack that maps all of (0,127,128,255) values "correctly" but not all the other values inbetween (and the expression is ugly).

makc commented 4 years ago

TBH I think what OP did (using onBeforeCompile to hack the shader) is the best solution. But unfortunately only few people would come up with it on their own.

Mugen87 commented 4 years ago

Thanks for the summary. In any event, it stays a hack and as long as other engines produce the same output as current three.js (see https://github.com/mrdoob/three.js/issues/18565#issuecomment-583324538), I do not vote to add it to the core.

cptSwing commented 4 years ago

Do you feel it should be merged

it makes sense if it is established that all the normal maps out there use 128 for zero. if some round down to 127 instead, there again will be a problem, and I did not test enough normal maps on pixel level to be sure.

This paragraph (and especially the forum post linked to on Polycount) seems to establish that the 128 is the norm for 'unsigned' normal maps: http://wiki.polycount.com/wiki/Normal_map#Flat_Color

I'm not registered to look at Unreals code nor intellectually proficient to understand what Unity is doing here, but both generally offer bug-free normal map mirroring from an artists perspective. Not sure Blender is a good benchmark here since it's a 'it'll get done if somebody wants to do it' type of product (no front!) ;)

edit: In substance designer/painter (likely number one texturing tools for 3d artists) 128/128/255 denotes 0 as well

Mugen87 commented 4 years ago

I've tested NormalTest_naive.glb.zip in Babylon.js and Sketchfab (which are more comparable to three.js than Unity and Unreal).

Babylon.js:

image

Sketchfab:

image

Same as three.js.

cptSwing commented 4 years ago

I've tested NormalTest_naive.glb.zip in Babylon.js and Sketchfab (which are more comparable to three.js than Unity and Unreal).

Thank you for testing!

From a 3d artists perspective, I'd argue that since most game-ready/realtime assets nowadays are authored with 128/128/255 as 0, three.js should follow this convention. This is the case for the Substance software packages as well as Quixels tools.

Mind, getting a mirroring seam to disappear completely on a 100% reflective surface depends on other things as well (16bit normal maps or not, compression type and strength, dithering, mipmap level etc)

This is an obj with mirroring seam down the middle, authored in substance painter to 127 R,G top half, and 128 bottom half

Marmoset Toolbag 2, a realtime asset viewer from 2015 (16bit Tiff normal map): image

And Unity 2019.3 (8bit BC7 normal map - sorry this screenshot is a bit shit): image

makc commented 4 years ago

@cptSwing what does marmoset do with 0 and 255 values? if you place the zero at 128 and keep using simlpe linear function, you have two options: a) 255 mapping to something less than 1, or b) 0 mapping to something less than -1 (well, ok, you could hav both ends wrong, too - but I assume we would want to get aT least one right)

WestLangley commented 4 years ago

Assuming map is pre-normalized to [ 0, 1 ] by division by 255:

mapN = ( map > 128 / 255 ) ? map * 255 / 127 - 128 / 127 : map * 255 / 128 - 1;

Or, assuming map is in [ 0, 255 ]:

mapN = ( map > 128 ) ? ( map - 128 ) / 127 : map / 128 - 1;
cptSwing commented 4 years ago

@cptSwing what does marmoset do with 0 and 255 values? if you place the zero at 128 and keep using simlpe linear function, you have two options: a) 255 mapping to something less than 1, or b) 0 mapping to something less than -1 (well, ok, you could hav both ends wrong, too - but I assume we would want to get aT least one right)

Good question. I dug around a little but couldn't find anything that stuck out. I would assume the 16bit normal map helps though?? Also scoured Unitys shaders since their implementation of BC compression works with 8bits as well, but quickly realized I was in way over my head, haha.

I found this blog post though, breaking down the two methods for unpacking normals and their pros/cons: http://www.aclockworkberry.com/normal-unpacking-quantization-errors/

makc commented 4 years ago

@WestLangley -s hack would work, too. One could even do this Screen Shot 2020-04-02 at 14 32 09

and have a smooth derivative at 0.