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

metallic not rendered well #62

Closed
tigrazone opened this issue Aug 17, 2024 · 10 comments
Closed

metallic not rendered well #62

tigrazone opened this issue Aug 17, 2024 · 10 comments

Comments

@tigrazone
Copy link

tigrazone commented Aug 17, 2024

image

same scene rendered by vk_raytrace, Disney
image

scene taken from https://github.com/h3r2tic/rtoy-samples/tree/main/assets/meshes/pica_pica_-_mini_diorama_01

which variant of brdf implementation is better and physically correct?

@mklefrancois
Copy link
Contributor

The vk_gltf_renderer has the physically correct PBR, not an approximation like vk_raytrace, but the problem with the bolts seems to be the missing tangents. Unlike vk_raytrace, attributes are not always generated on scene load, but passed directly to the render engine. If the attribute is missing and requested, it uses a default value, or in the case of tangents, tries to create something on the fly.

In this case, the tangents aren't created correctly and appear to be NULL.

(Debug View: tangents)
image

Thank you for reporting the problem with this scene. We'll see what we can do to fix this and still keep the amount of data generated to a minimum.

@tigrazone
Copy link
Author

is it good idea to calculate absent tangents with simple formula

mat3 get_onb(vec3 n) {
  // from Spencer, Jones "Into the Blue", eq(3)
  vec3 tangent = normalize(cross(n, vec3(-n.z, n.x, -n.y)));
  vec3 bitangent = cross(n, tangent);
  return mat3(tangent, bitangent, n);
}

code taken from https://github.com/DassaultSystemes-Technology/dspbr-pt/blob/e7cfa6e9aab2b99065a90694e1f58564d675c1a4/packages/lib/shader/utils.glsl#L119

@mklefrancois
Copy link
Contributor

The computation of tangent vectors is not problematic; however, the scene contains tangent data, some of which are represented as (0,0,0,1) vectors. The validity of such null tangents is uncertain with respect to the glTF specification.

For example, the following as issues:

  • primitive ID 4
  • tangent index 215

Note: vk_raytracing project goes over all the data and fixes invalid values, before uploading to the GPU.

It would be possible for example to do the following in the get_hit

    if(tng[0].xyz == vec3(0, 0, 0) || tng[1].xyz == vec3(0, 0, 0) || tng[2].xyz == vec3(0, 0, 0))
    {
      vec4 t = makeFastTangent(hit.nrm);
      tng[0] = t;
      tng[1] = t;
      tng[2] = t;
    }

But I'm not sure if the renderer should fix.

@tigrazone
Copy link
Author

it helps. thank you

@tigrazone
Copy link
Author

tigrazone commented Aug 19, 2024

I do it with precission fix

  // Tangent - Bitangent
  vec4 tng[3];
  bool needFixTangent = true;
  if(hasVertexTangent(renderPrim))
  {
    tng[0] = getVertexTangent(renderPrim, triangleIndex.x);
    tng[1] = getVertexTangent(renderPrim, triangleIndex.y);
    tng[2] = getVertexTangent(renderPrim, triangleIndex.z);
	needFixTangent = isZEROv(tng[0]) || isZEROv(tng[1]) || isZEROv(tng[2]);
  }
  
  if(needFixTangent)
  {
    vec4 t = makeFastTangent(hit.nrm);
    tng[0] = t;
    tng[1] = t;
    tng[2] = t;
  }

where isZERO() is

#define NEARzero 0.00001f
#define isZERO(x) ((x)>-NEARzero && (x)<NEARzero)
#define isZEROv(v) ( isZERO((v).x) && isZERO((v).y) && isZERO((v).z) )

@mklefrancois
Copy link
Contributor

This is the version that might be committed

  // Tangent - Bitangent
  vec4 tng[3];
  bool hasTangent = hasVertexTangent(renderPrim);

  if(hasTangent)
  {
    tng[0] = getVertexTangent(renderPrim, triangleIndex.x);
    tng[1] = getVertexTangent(renderPrim, triangleIndex.y);
    tng[2] = getVertexTangent(renderPrim, triangleIndex.z);

    float lenSq = dot(tng[0].xyz, tng[0].xyz) * dot(tng[1].xyz, tng[1].xyz) * dot(tng[2].xyz, tng[2].xyz);
    hasTangent  = lenSq > EPSILON;
  }

  if(!hasTangent)
  {
    vec4 t = makeFastTangent(hit.nrm);
    tng[0] = t;
    tng[1] = t;
    tng[2] = t;
  }

@tigrazone
Copy link
Author

Maybe better to do this precalc on CPU side for faster rendering?

@tigrazone tigrazone reopened this Aug 19, 2024
@mklefrancois
Copy link
Contributor

The math is "free" on a fast GPU, but changing the scene on the fly bothers me. This could be a choice of the artist, even if a tangent of length != 1.0 is not legal. So basically there is no bug in the vk_gltf_renderer, it is just the scene that is not correct.

I'm looking into the possibility of adding a tool to fix the scene, then the user could save it again to avoid having to fix it every time.

@tigrazone
Copy link
Author

Why not use only calculated tangents?

@mklefrancois
Copy link
Contributor

To close this issue: There is now a tool in vk_gltf_renderer to create/fix tangents. Once fixed, the scene can be saved and does not need any further corrections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants