SurfaceOrientation doesn't duplicate vertices when generating flat normals

[cpheinrich]

Describe the bug
I am having two (separate I believe) issues rendering some meshes in Filament. I'm only opening one issue because they are both related to the same mesh type. We generate many meshes of this type, and the issue is not confined to this specific mesh, but all meshes of this type. First, this is what the mesh looks like in scenekit:

Issue 1

The Filament directional lights only illuminate parts of the scene. This is clear if I light the scene with only a directional light. This issue appears in our viewer as well as the sample-gltf iOS viewer. As you can see a large chunk of the ground plane is not illuminated by the light (maybe there is a setting to fix this, but I played with all that seemed relevant)

At first I thought it was maybe an issue on our side with the triangulation, but as you can see in meshlab the ground plane only has two triangles and appear to be unrelated to the illumination boundaries

*** Issue 2 ***

The colors for the mesh are pretty far off from what they are in all other viewers (SceneKit, MeshLab, ThreeJS). In particular they appear to be much less saturated. I have tried adjusting the colors and intensity of the lights but this did not fix it. Here is what it looks like in Filament when I adjusted the directional and indirect lights to be as good as I could make them

To Reproduce

Load this .glb in filament and observe the issues:

house.glb.zip

These were my exact lighting settings:

    math::float3 harmonics[1];
    harmonics[0] = math::float3(1.0f, 1.0f, 1.0f);
    indirect_light_ = IndirectLight::Builder().irradiance(1, harmonics).intensity(24000).build(*engine_);
    if (indirect_light_ == nullptr) {
      throw std::runtime_error("Creating a filament indirect light failed!");
    }
    scene_->setIndirectLight(indirect_light_);

    // Lights and shadows for room mode captures
    utils::Entity light = utils::EntityManager::get().create();
    LightManager::ShadowOptions shadowOptions;
    shadowOptions.shadowCascades = 3;
    shadowOptions.mapSize = 4096;
    shadowOptions.vsm.blurWidth = 120;
    shadowOptions.vsm.msaaSamples = 16;

    LightManager::Builder(LightManager::Type::DIRECTIONAL)
        .color(Color::toLinear<ACCURATE>({0.98f, 0.9f, 0.86f}))
        .intensity(38000)
        .direction({0.2, -1, -0.3})
        .castShadows(true)
        .shadowOptions(shadowOptions)
        .build(*engine_, light);
    scene_->addEntity(light);

Expected behavior
A clear and concise description of what you expected to happen.

Screenshots
If applicable, add screenshots to help explain your problem.

Logs
If applicable, copy logs from your console here. Please do not
use screenshots of logs, copy them as text.

Desktop (please complete the following information):

• OS: tested on iOS, Mac, Android and WebGL
• GPU: All tested
• Backend: Tested on OpenGL, Metal and WebGL

Smartphone (please complete the following information):

• Device: [e.g. Pixel 2]
• OS: [e.g. Android Pie 9.0]

Additional context
Add any other context about the problem here.

[romainguy]

The lighting problem happens in our desktop gltf_viewer too. It definitely looks like an issue with the mesh or how the mesh is loaded. The problem is fixed if I re-export your glb using Blender:

house_fixed.glb.zip

For the colors, make sure you are using the same tone mapper. Filament defaults to an ACES tone mapper, but three.js for instance uses a linear tone mapper by default I believe. You can change the tone mapper via the ColorGrading API.

[romainguy]

Here's a better screenshot with light intensities and colors closer to your setup (and linear tone mapping):

[cpheinrich]

Ok, thanks for trying out the blender import/export. Although its still confusing that the issue doesn't show up in other viewers, so maybe its some issue at the intersection of mesh + viewer. I can see if there is anything else we can do in that regard though. The blender exported one does look much better though, thanks for trying that!

Re colors, yes we are already using a linear color mapper, and when I tried using the default one the colors were even less saturated. I will play with the other tone mapper settings to see if it makes a difference. We can also change the RGB colors in the gltf to compensate I suppose.

[romainguy]

If you are using the same tone mapper as in the other renderers, it should look very similar. The only other thing I can think about is that Filament also outputs in properly encoded sRGB (but it should be the case of those other renderers as well). Of course also making sure you use the same light and exposure setup (you can make FIlament behave like non-photometric based renderers by calling Camera.setExposure(1f) and then the light intensities are relative, so you'd set the directional light's intensity to 1 for instance instead of 38,000. Also make sure your light colors are correct: all Filament APIs expect to be fed colors in "linear sRGB" but we provide conversion APIs (like you did in your sample).

[cpheinrich]

@romainguy after further investigating, we found that importing and exporting from blender only fixes the issue if we enable the 'save normals' option in Blender. When we generate the gltf we do not save the normals to keep file size small and because they are redundant, so it seems the lack of normals is likely the issue.

We can modify our gltf generator to generate the normals and save them to disk, but it seems that filament is either (i) not computing normals or (ii) computing them incorrectly. Are there any options we could enable during gltf loading or after to recompute the normals at asset load-time? Any other suggestions? I looked but didn't see anything. cc @cdcseacave

[romainguy]

Filament does generate normals if they are missing. They are various ways of doing this and it seems that this particular mesh doesn't work well with our approach. We build flat normals when only positions and indices are supplied:

SurfaceOrientation* OrientationBuilderImpl::buildWithFlatNormals() {
    float3* normals = new float3[vertexCount];
    for (size_t a = 0; a < triangleCount; ++a) {
        const uint3 tri = triangles16 ? uint3(triangles16[a]) : triangles32[a];
        assert_invariant(tri.x < vertexCount && tri.y < vertexCount && tri.z < vertexCount);
        const float3 v1 = positions[tri.x];
        const float3 v2 = positions[tri.y];
        const float3 v3 = positions[tri.z];
        const float3 normal = normalize(cross(v2 - v1, v3 - v1));
        normals[tri.x] = normal;
        normals[tri.y] = normal;
        normals[tri.z] = normal;
    }
    this->normals = normals;
    SurfaceOrientation* result = buildWithNormalsOnly();
    this->normals = nullptr;
    delete[] normals;
    return result;
}

BTW it might be better in general to provide normals (and tangents maybe) but use mesh compression to still produce small gltf file but with enough data to make loading faster.

[romainguy]

It looks like for your particular case that we may need to generate smooth shading normals.

[romainguy]

I created a repro case with just the floor. It should work with flat normals too.

[romainguy]

At first glance the normals generated for the floor look correct:

< -0.000000, 1.000000, 0.000000 >
< 0.000000, 1.000000, 0.000000 >
< 0.000000, -1.000000, 0.000000 >
< 0.000000, -1.000000, 0.000000 >
< 0.000000, 0.000000, 1.000000 >
< 0.000000, 0.000000, 1.000000 >
< -1.000000, -0.000000, -0.000000 >
< -1.000000, 0.000000, 0.000000 >
< 0.000000, 0.000000, -1.000000 >
< 0.000000, 0.000000, -1.000000 >
< 1.000000, 0.000000, -0.000000 >
< 1.000000, -0.000000, 0.000000 >

But here are the normals visualized as RGB:

I don't understand how this can be the result given the mesh is made of 2 triangles for the quad.

[romainguy]

Alright, I figured out the problem.

[romainguy]

The problem is that per the glTF spec we apply flat shading when no normals are provided. Unfortunately the code that does that does not duplicate shared vertices, which means we share normals and tangents across faces that are not coplanar.

[romainguy]

This file contains a glb that isolates the issue:
flat_normals_bug.zip

Flat normals are computed per triangle but we don't duplicate vertices. So adjacent triangles erase each other's normals in SurfaceOrientation.cpp/OrientationBuilderImpl::buildWithFlatNormals(), which in turns creates degenerate tangent frames in buildWithNormalsOnly() in the same file. The fix is to make buildWithFlatNormals() properly duplicate shared vertices when generating the flat normals.

The current workaround is to generate glTF files with normals in them.

[cpheinrich]

Ok, thanks for investigating! We've implemented normal generation on our side and now it work as expected. Regarding the colors issue in my previous post, I actually think this is a non-issue now since the rendered colors in the filament viewer are actually closer to the ones in the glb than in the other renderers

[romainguy]

I'll reopen it so we can fix this issue on our end

[cdcseacave]

Lack of duplicating the vertices is not the issue here (though that could help in general): our mesh should have the vertices already duplicated for the 90 degrees surfaces, at least for the walls and floor (not for the objects). We compute the normals as you showed you compute them in filament, and that seemed to fix the issue as can be seen in the screenshot above. So in my opinion there must be some vertex merging happening during GLB loading, or some other problem.

[romainguy]

The problem goes away when removing the sides of the floor so no vertices are shared and vertices aren't duplicated by the time the normals/tangent frames generation code runs. We don't do any mesh optimization ourselves and I'd be surprised if cgltf does.

[cdcseacave]

Indeed, the floor is triangulated differently from the walls, I checked again now, and the vertices are shared. It is strange though that even with this simple normal estimation if done by me and saved into the GLB the surface lights correctly, and not when the normals are estimated by filament. This simple normal estimation does not seem to work well for the rest of the objects, so I will try some other methods.

[romainguy]

@poweifeng Do we need more to close this issue?

[poweifeng]

@poweifeng Do we need more to close this issue?

Yes, sorry. This has been neglected. Just one or two more changes needed.