Conversation
There was a problem hiding this comment.
Pull request overview
Updates Obj2Tiles’ OBJ/MTL parsing robustness and expands the automated test suite to cover many previously untested edge-cases across the decimation (ObjMesh), splitting (MeshUtils), and glTF conversion (ObjParser) paths, while aligning publish profiles with the repo’s net10.0 target.
Changes:
- Update
MeshUtils.LoadMeshto wrap out-of-range UVs, triangulate quads/n-gons, and skiplline elements. - Add broad NUnit/Shouldly coverage + fixtures for OBJ parsing edge cases, multi-axis splitting, and recursive splitting.
- Update platform publish profiles to output to
net10.0publish directories.
Reviewed changes
Copilot reviewed 45 out of 47 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Obj2Tiles/Properties/PublishProfiles/win-x64.pubxml | Publish output paths/framework updated to net10.0. |
| Obj2Tiles/Properties/PublishProfiles/win-arm64.pubxml | Publish output paths/framework updated to net10.0. |
| Obj2Tiles/Properties/PublishProfiles/osx-x64.pubxml | Publish output paths/framework updated to net10.0. |
| Obj2Tiles/Properties/PublishProfiles/linux-x64.pubxml | Publish output paths/framework updated to net10.0. |
| Obj2Tiles/Properties/PublishProfiles/linux-arm64.pubxml | Publish output paths/framework updated to net10.0. |
| Obj2Tiles.Library/Geometry/MeshUtils.cs | Splitting-stage OBJ parser: UV wrapping, n-gon triangulation, skip l, fix vn recognition. |
| Obj2Tiles.Test/Obj2Tiles.Test.csproj | Add Obj2Gltf reference + copy new OBJ fixtures to output. |
| Obj2Tiles.Test/ObjParserGltfTests.cs | New tests for Obj2Gltf’s ObjParser.Parse behavior (triangulation, normals, colors, etc.). |
| Obj2Tiles.Test/ObjMeshParsingTests.cs | New tests for decimation-stage ObjMesh.ReadFile parsing edge cases. |
| Obj2Tiles.Test/TestData/triangle.obj | New OBJ fixture (simple triangles). |
| Obj2Tiles.Test/TestData/triangle-normals.obj | New OBJ fixture (f v//vn format). |
| Obj2Tiles.Test/TestData/scientific-notation.obj | New OBJ fixture (scientific notation coordinates). |
| Obj2Tiles.Test/TestData/quad-simple.obj | New OBJ fixture (quad face). |
| Obj2Tiles.Test/TestData/quad-mesh.obj | New OBJ fixture (quad for ObjMesh triangulation). |
| Obj2Tiles.Test/TestData/ngon.obj | New OBJ fixture (pentagon n-gon). |
| Obj2Tiles.Test/TestData/negative-idx.obj | New OBJ fixture (negative indices). |
| Obj2Tiles.Test/TestData/empty.obj | New empty OBJ fixture. |
| Obj2Tiles.Test/TestData/degenerate-faces.obj | New OBJ fixture (degenerate face). |
| Obj2Tiles.Test/TestData/cube2-normals.obj | New OBJ fixture (normals + uvs + materials). |
| Obj2Tiles.Test/TestData/cube-colors-3d.obj | New OBJ fixture (vertex colors). |
| Obj2Tiles.Test/TestData/cube-3d.obj | New OBJ fixture (3D cube triangles). |
| Obj2Tiles.Test/TestData/comments-and-blanks.obj | New OBJ fixture (comments/blank lines). |
| Obj2Tiles.Library.Test/Obj2Tiles.Library.Test.csproj | Copy new Library.Test fixtures to output. |
| Obj2Tiles.Library.Test/ObjParsingTests.cs | New tests for MeshUtils.LoadMesh corner cases (Issues #35/#60/#64 focus). |
| Obj2Tiles.Library.Test/SplitMultiAxisTests.cs | New tests for split correctness across X/Y/Z + color preservation. |
| Obj2Tiles.Library.Test/RecurseSplitTests.cs | New tests for RecurseSplitXY/XYZ behaviors. |
| Obj2Tiles.Library.Test/MtlParsingTests.cs | New tests for Material.ReadMtl parsing and round-trips. |
| Obj2Tiles.Library.Test/BoundsTests.cs | New tests for bounds and Box3 split/center invariants. |
| Obj2Tiles.Library.Test/TestData/triangle-with-lines.obj | New fixture to validate skipping l elements. |
| Obj2Tiles.Library.Test/TestData/triangle-vt-vn.obj | New fixture for f v/vt/vn faces. |
| Obj2Tiles.Library.Test/TestData/triangle-vt-vn.mtl | New fixture MTL for vt/vn triangle. |
| Obj2Tiles.Library.Test/TestData/triangle-uv-negative.obj | New fixture for negative/out-of-range UV wrapping. |
| Obj2Tiles.Library.Test/TestData/triangle-uv-negative.mtl | New fixture MTL for UV wrapping test. |
| Obj2Tiles.Library.Test/TestData/triangle-normals.obj | New fixture (f v//vn). |
| Obj2Tiles.Library.Test/TestData/scientific-notation.obj | New fixture for scientific notation parsing. |
| Obj2Tiles.Library.Test/TestData/quad.obj | New fixture for quad triangulation. |
| Obj2Tiles.Library.Test/TestData/only-vertices.obj | New fixture (vertices only, no faces). |
| Obj2Tiles.Library.Test/TestData/ngon.obj | New fixture (n-gon triangulation). |
| Obj2Tiles.Library.Test/TestData/multi-material.obj | New fixture (multi-material faces). |
| Obj2Tiles.Library.Test/TestData/multi-material.mtl | New fixture MTL (two materials). |
| Obj2Tiles.Library.Test/TestData/mtllib-missing.obj | New fixture (missing MTL reference). |
| Obj2Tiles.Library.Test/TestData/mtllib-empty.obj | New fixture (empty mtllib line). |
| Obj2Tiles.Library.Test/TestData/empty.obj | New empty OBJ fixture (Library.Test). |
| Obj2Tiles.Library.Test/TestData/degenerate-faces.obj | New fixture (degenerate faces). |
| Obj2Tiles.Library.Test/TestData/cube-colors-3d.obj | New fixture (vertex colors). |
| Obj2Tiles.Library.Test/TestData/cube-3d.obj | New fixture (3D cube triangles). |
| Obj2Tiles.Library.Test/TestData/comments-and-blanks.obj | New fixture (comments/blank lines). |
| var hasTex = faceVerts[0].Length > 1 && faceVerts[0][1].Length > 0; | ||
|
|
There was a problem hiding this comment.
In the n-gon triangulation path, hasTex is determined only from the first face vertex. If the first vertex has a vt index but a later vertex omits it (e.g., mixed v/vt and v//vn), the subsequent int.Parse(faceVerts[fi][1]) will throw. Consider computing hasTex as “all vertices have a non-empty vt component”, and either fall back to non-textured faces or throw a clearer format error when the face has inconsistent indices.
| var hasTex = faceVerts[0].Length > 1 && faceVerts[0][1].Length > 0; | |
| // Determine whether all vertices have texture coordinates. | |
| var anyHasTex = false; | |
| var allHaveTex = true; | |
| for (var fi = 0; fi < faceVerts.Length; fi++) | |
| { | |
| var hasVt = faceVerts[fi].Length > 1 && faceVerts[fi][1].Length > 0; | |
| anyHasTex |= hasVt; | |
| allHaveTex &= hasVt; | |
| } | |
| if (anyHasTex && !allHaveTex) | |
| throw new System.FormatException("OBJ face has inconsistent texture coordinate indices (mixed v/vt and v//vn or missing vt) which is not supported."); | |
| var hasTex = allHaveTex; |
| for (var fi = 1; fi < faceVerts.Length - 1; fi++) | ||
| { | ||
| var fv0 = int.Parse(faceVerts[0][0]) - 1; | ||
| var fv1 = int.Parse(faceVerts[fi][0]) - 1; | ||
| var fv2 = int.Parse(faceVerts[fi + 1][0]) - 1; |
There was a problem hiding this comment.
The fan-triangulation loop re-parses faceVerts[0][0] (and faceVerts[0][1] when textured) on every iteration. Parsing fv0/ft0 once outside the loop would avoid repeated int.Parse work on large polygons and keep this path closer in cost to the triangle case.
| // BUG: Issue #36 — the Converter.cs ignores Kd when no texture is present, | ||
| // producing grey materials in glTF. This test verifies the MTL parser works correctly; | ||
| // the downstream glTF conversion bug is separate. |
There was a problem hiding this comment.
Addresses #36 #54 #35 #60 #64