KhronosGroup / glTF-Sample-Assets

To store all models and other assets related to glTF
255 stars 30 forks source link

Correct and clean up PNG normals textures #112

Closed erich666 closed 3 months ago

erich666 commented 3 months ago

Does not include any JPG or KTX versions of these texture, nor any .GLB files including the textures - those would need to be recompressed. Discussion begun on old Models repo, see https://github.com/KhronosGroup/glTF/issues/2368 for background, etc.

I used my new open-source tool, NormalTextureProcessor to check the normals textures in this repo for compliance with the glTF spec.

My notes on the files corrected, and not corrected, are attached. Basically, almost all PNG normals textures have been cleaned up and are in this PR, JPEG normals textures not touched (see notes below), KTX not touched, and GLB files that include normals textures not updated. The JPEGs are lossy, so it's tricky to clean these up. I don't know how to form KTX textures or GLBs, though am happy to be advised and add these to the PR if I must.

Notes attached in notes.txt. notes.txt

erich666 commented 3 months ago

One newer KTX normals texture file that also needs to be updated: \Github\glTF-Sample-Assets\Models\AnisotropyBarnLamp\glTF-KTX-BasisU\AnisotropyBarnLamp_normalbump.ktx2

erich666 commented 3 months ago

Hmmm, I'd like to close this PR, as I now think it's too aggressive. I will resubmit it later. Some of the problems found are minor precision glitches in the original content. Judging by less-fussy criteria, about 11 of the normals textures are OK: glTF_asset_normals\camera_camera_Normal.png glTF_asset_normals\camera_tripod_Normal.png glTF_asset_normals\ClearCoatCarPaint_Normal.png glTF_asset_normals\Fabric_normal.png glTF_asset_normals\FlightHelmet_Materials_LensesMat_Normal.png glTF_asset_normals\GlamVelvetSofa_normal.png glTF_asset_normals\Heights_1d_Normals_v2.png glTF_asset_normals\material1_normal.png glTF_asset_normals\NormalTangentTest_Normal.png glTF_asset_normals\TwoSidedPlane_Normal.png glTF_asset_normals\WindowGlass_Normal.png

Others are borderline, some textures are quite bad. For example, the abalone normals texture is pretty off, here's the corrected version next to the original, showing the difference: image

Attached is a new analysis dump for all normals textures in Assets and what the analyzer found. analysis.txt

erich666 commented 3 months ago

I should note that file https://github.com/KhronosGroup/glTF-Sample-Assets/blob/main/Models/TextureTransformMultiTest/glTF/TestMap_Normal.png is still quite wrong. It maps Z from 0 to 1, instead of what the spec says, -1 to 1.

Analysis, noting how it's a perfect Z 0 to 1 texture, which is not what glTF wants: Image file 'TestMap_Normal.png' is a Z-zero normals texture, with Z ranging from 0.0 to 1.0. The X and Y coordinates always form vectors that are length 1.0 or less. The texture has all good z-values. Normal lengths found in the file, assuming Z 0 to 1: minimum 0.994526 and maximum 1.00553 (these should be near 1.0). The lowest Z value found was 0.317647 (to Z-Zero) Average XYZ values, assuming Z 0 to 1: X -0.0022086, Y -0.00215911, Z 0.839846 XYZ values ranges, assuming Z 0 to 1: X -0.945098 to 0.945098 Y -0.945098 to 0.945098 Z 0.317647 to 1 Color channels range: r 7 to 248; g 7 to 248; b 81 to 255.

Original discussion here, https://github.com/KhronosGroup/glTF-Sample-Models/issues/364, closed for reasons unclear to me.

javagl commented 3 months ago

I should note that file [...] is still quite wrong. It maps Z from 0 to 1, instead of what the spec says, -1 to 1.

Original discussion here, https://github.com/KhronosGroup/glTF-Sample-Models/issues/364, closed for reasons unclear to me.

It was closed because this issue is tracked in https://github.com/KhronosGroup/glTF-Sample-Assets/issues/41#issuecomment-1817221967 - but I'm surprised to see that now...

There have been two PRs ( 365 and 367 ) that went into the glTF-Sample-Models repo on Jan 3, 2023.

The glTF-Sample-Assets repo should actually contain a later state of the models (i.e. one that was taken from the ...Models repo after this date). But it seems to contain an older version of that model. So I wonder where the old file slipped in (and which other PRs might have been lost...).

@DRx3D Any idea why an older version of the model could have ended up in the glTF-Sample-Assets repo?

(We should figure out why this happened. In the meantime, that (single) model could be fixed via https://github.com/KhronosGroup/glTF-Sample-Assets/pull/113 , because the linked PR already contained the updated PNG and GLB files)

erich666 commented 3 months ago

One thing that also should happen if and when the new TestMap_Normal.png is put into place is that the .GLB file in glTF-Sample-Assets\Models\TextureTransformMultiTest\glTF-Binary, i.e., here, should also be made again, with the new texture in it.

javagl commented 3 months ago

@erich666 This should be part of the open PR, as of https://github.com/KhronosGroup/glTF-Sample-Assets/pull/113/files . The GLB that is included there is probably the one that was provided in https://github.com/KhronosGroup/glTF-Sample-Models/pull/365#issuecomment-1312091900 (I just took it from the corresponding PR), with the direct link to that file being https://github.com/KhronosGroup/glTF-Sample-Assets/raw/2cce622892cb42793d71b671339003c29b895e0d/Models/TextureTransformMultiTest/glTF-Binary/TextureTransformMultiTest.glb , which contains the same texture as the one in the glTF directory.

erich666 commented 3 months ago

Updated PR here: https://github.com/KhronosGroup/glTF-Sample-Assets/pull/114