Merge pull request #541 from CesiumGS/no-normals-problems

Fix loading problems for models without normals

Author: kring

CHANGES.md

@@ -6,6 +6,8 @@
 
 - Fixed a bug that could cause a crash on AppDomain reloads.
 - Fixed a bug that could cause a crash or incorrect textures when multiple `Cesium3DTileset` tiles referenced the same image by URL.
+- Fixed a bug that could cause incorrect colors in a model that did have vertex colors but did not have normals.
+- Fixed a bug that could cause a hang when attempting to load a model with UINT16 indices where generating flat normals required more than 2^16 vertices.
 - Fixed a bug in the Abseil vcpkg overlay port that could cause linker errors on some systems.
 
 ## v1.14.0 - 2024-12-02

Tests/TestCesium3DTileset.cs

@@ -156,4 +156,47 @@ public IEnumerator SampleHeightMostDetailedFailsIfTilesetFailsToLoad()
 
         Assert.IsTrue(result.warnings[0].Contains("failed to load"));
     }
+
+    [UnityTest]
+    public IEnumerator UpgradeToLargerIndexType()
+    {
+        // This tileset has no normals, so we need to generate flat normals for it.
+        // When we do that, an index buffer will need to change from uint16 to uint32.
+        GameObject goGeoreference = new GameObject();
+        goGeoreference.name = "Georeference";
+        CesiumGeoreference georeference = goGeoreference.AddComponent<CesiumGeoreference>();
+
+        GameObject goTileset = new GameObject();
+        goTileset.name = "Snowdon Towers No Normals";
+        goTileset.transform.parent = goGeoreference.transform;
+
+        Cesium3DTileset tileset = goTileset.AddComponent<Cesium3DTileset>();
+        CesiumCameraManager cameraManager = goTileset.AddComponent<CesiumCameraManager>();
+        tileset.ionAccessToken = Environment.GetEnvironmentVariable("CESIUM_ION_TOKEN_FOR_TESTS") ?? "";
+        tileset.ionAssetID = 2887128;
+
+        georeference.SetOriginLongitudeLatitudeHeight(-79.88602625, 40.02228799, 222.65);
+
+        GameObject goCamera = new GameObject();
+        goCamera.name = "Camera";
+        goCamera.transform.parent = goGeoreference.transform;
+
+        Camera camera = goCamera.AddComponent<Camera>();
+        CesiumGlobeAnchor cameraAnchor = goCamera.AddComponent<CesiumGlobeAnchor>();
+
+        cameraManager.useMainCamera = false;
+        cameraManager.useSceneViewCameraInEditor = false;
+        cameraManager.additionalCameras.Add(camera);
+
+        camera.pixelRect = new Rect(0, 0, 128, 96);
+        camera.fieldOfView = 60.0f;
+        cameraAnchor.longitudeLatitudeHeight = new double3(-79.88593359, 40.02255615, 242.0224);
+        camera.transform.LookAt(new Vector3(0.0f, 0.0f, 0.0f));
+
+        // Make sure we can load all tiles successfully.
+        while (tileset.ComputeLoadProgress() < 100.0f)
+        {
+            yield return null;
+        }
+    }
 }

native~/Runtime/src/UnityPrepareRendererResources.cpp

@@ -161,11 +161,11 @@ template <typename TIndex> struct CopyVertexColors {
     bool success = true;
     if (duplicateVertices) {
       for (size_t i = 0; success && i < vertexCount; ++i) {
-        if (i >= colorView.size()) {
+        TIndex vertexIndex = indices[i];
+        if (vertexIndex < 0 || vertexIndex >= colorView.size()) {
           success = false;
         } else {
           Color32& packedColor = *reinterpret_cast<Color32*>(pWritePos);
-          TIndex vertexIndex = indices[i];
           success = CopyVertexColors::convertColor(
               colorView[vertexIndex],
               packedColor);
@@ -347,6 +347,43 @@ void loadPrimitive(
     return;
   }
 
+  const CesiumGltf::Material* pMaterial =
+      Model::getSafe(&gltf.materials, primitive.material);
+
+  primitiveInfo.isUnlit =
+      pMaterial && pMaterial->hasExtension<ExtensionKhrMaterialsUnlit>();
+
+  bool hasNormals = false;
+  bool computeFlatNormals = false;
+  auto normalAccessorIt = primitive.attributes.find("NORMAL");
+  AccessorView<UnityEngine::Vector3> normalView;
+  if (normalAccessorIt != primitive.attributes.end()) {
+    normalView =
+        AccessorView<UnityEngine::Vector3>(gltf, normalAccessorIt->second);
+    hasNormals = normalView.status() == AccessorViewStatus::Valid;
+  } else if (
+      !primitiveInfo.isUnlit && primitive.mode != MeshPrimitive::Mode::POINTS) {
+    computeFlatNormals = hasNormals = true;
+  }
+
+  // Check if  we need to upgrade to a large index type to accommodate the
+  // larger number of vertices we need for flat normals.
+  if (computeFlatNormals && indexFormat == IndexFormat::UInt16 &&
+      indexCount >= std::numeric_limits<uint16_t>::max()) {
+    loadPrimitive<uint32_t>(
+        meshData,
+        primitiveInfo,
+        gltf,
+        node,
+        mesh,
+        primitive,
+        transform,
+        indicesView,
+        IndexFormat::UInt32,
+        positionView);
+    return;
+  }
+
   meshData.SetIndexBufferParams(indexCount, indexFormat);
   const Unity::Collections::NativeArray1<TIndex>& dest =
       meshData.GetIndexData<TIndex>();
@@ -399,25 +436,7 @@ void loadPrimitive(
   descriptor[numberOfAttributes].stream = streamIndex;
   ++numberOfAttributes;
 
-  const CesiumGltf::Material* pMaterial =
-      Model::getSafe(&gltf.materials, primitive.material);
-
-  primitiveInfo.isUnlit =
-      pMaterial && pMaterial->hasExtension<ExtensionKhrMaterialsUnlit>();
-
   // Add the NORMAL attribute, if it exists.
-  bool hasNormals = false;
-  bool computeFlatNormals = false;
-  auto normalAccessorIt = primitive.attributes.find("NORMAL");
-  AccessorView<UnityEngine::Vector3> normalView;
-  if (normalAccessorIt != primitive.attributes.end()) {
-    normalView =
-        AccessorView<UnityEngine::Vector3>(gltf, normalAccessorIt->second);
-    hasNormals = normalView.status() == AccessorViewStatus::Valid;
-  } else if (
-      !primitiveInfo.isUnlit && primitive.mode != MeshPrimitive::Mode::POINTS) {
-    computeFlatNormals = hasNormals = true;
-  }
   if (hasNormals) {
     assert(numberOfAttributes < MAX_ATTRIBUTES);
     descriptor[numberOfAttributes].attribute = VertexAttribute::Normal;