nvpro-samples / nvpro_core

shared source code and resources needed for the samples to run
Apache License 2.0
489 stars 114 forks source link

metallic not rendered well #62

Closed tigrazone closed 2 months ago

tigrazone commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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:

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 commented 2 months ago

it helps. thank you

tigrazone commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

mklefrancois commented 2 months ago

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 commented 2 months ago

Why not use only calculated tangents?

mklefrancois commented 2 months ago

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.