Probably incorrect vertex index in setUniformNormals function

[TeddyProg]

In setUniformNormals we are iterating through vertices, but use size of indices array. So we can go out of boundaries in vertices array. https://github.com/CesiumGS/cesium-unreal/blob/84ba3a121b45d8497ac2c9a9fd7fefebe32fe987/Source/CesiumRuntime/Private/CesiumGltfComponent.cpp#L287C16-L287C16

[TeddyProg]

Kinda similar thing here (https://github.com/CesiumGS/cesium-unreal/blob/84ba3a121b45d8497ac2c9a9fd7fefebe32fe987/Source/CesiumRuntime/Private/CesiumGltfComponent.cpp#L298) for (int i = 0; i < indices.Num(); i += 3) { FStaticMeshBuildVertex& v0 = vertices[i]; FStaticMeshBuildVertex& v1 = vertices[i + 1]; FStaticMeshBuildVertex& v2 = vertices[i + 2]; //... It probably should be: for (int i = 0; i < indices.Num(); i += 3) { FStaticMeshBuildVertex& v0 = vertices[indices[i]]; FStaticMeshBuildVertex& v1 = vertices[indices[i + 1]]; FStaticMeshBuildVertex& v2 = vertices[indices[i + 2]]; //...

[csciguy8]

This does feel a little wrong.

static void setUniformNormals(
    const TArray<uint32_t>& indices,
    TArray<FStaticMeshBuildVertex>& vertices,
    TMeshVector3 normal) {
  for (int i = 0; i < indices.Num(); i++) {
    FStaticMeshBuildVertex& v = vertices[i];
    v.TangentX = v.TangentY = TMeshVector3(0.0f);
    v.TangentZ = normal;
  }
}

If the intent is to just set all tangent vectors for this vertex, not really sure why we are considering indices at all. Would think it should just be....

static void setUniformNormals(
    TArray<FStaticMeshBuildVertex>& vertices,
    TMeshVector3 normal) {
  for (int i = 0; i < vertices.Num(); i++) {
    FStaticMeshBuildVertex& v = vertices[i];
    ...
  }
}

[j9liu]

Thanks for the catch @TeddyProg ! We'd welcome a community contribution to fix this, if you have the bandwidth 😄

[kring]

This is weird and confusing, but probably not an actual bug. Both of those code paths are only traversed when the glTF has no normals. And when a glTF has no normals, we generate flat normals. Generating flat normals requires that multiple triangles never share a vertex, which in turn requires us to copy each vertex each time it is referenced in the index buffer. Long story short: the number of vertices and number of indices are the same.

[TeddyProg]

@kring Oh, I got it. Probably in second case everything is also correct as we have duplicated vertices

[j9liu]

Generating flat normals requires that multiple triangles never share a vertex, which in turn requires us to copy each vertex each time it is referenced in the index buffer. Long story short: the number of vertices and number of indices are the same.

But ultimately, the indices parameter isn't even needed, right? As @csciguy8 suggested it could still be written like this...

static void setUniformNormals(
    TArray<FStaticMeshBuildVertex>& vertices,
    TMeshVector3 normal) {
  for (int i = 0; i < vertices.Num(); i++) {
    FStaticMeshBuildVertex& v = vertices[i];
    ...
  }
}

...and still be correct, unless I'm misunderstanding how setUniformNormals works. Either way it's no longer an issue of "correctness", just some small confusion, and would be nice to document / remove the unnecessary parameter.