Repository navigation
Release 0.5.0: footprint packing, subdivision, sampling, volume/watertight/join/submesh, OBJ I/O - #11
Conversation
…ts by default PackFootprints (RectPacking.h) packs binary masks by their footprints instead of their bounding rectangles: bottom-left first fit of the block-quantized masks over per-row free intervals, both orientations (winding-preserving rotation), pages tried at three widths and cropped to their content. openMVS's texturing moves its patch packer here. PackAtlas rasterizes every chart's UV triangles (conservative raster), grows them by the gutter and packs the footprints, also in the fit-to-resolution search (AtlasParams::packFootprints, default on). Triangle coverage 0.291 -> 0.502 on mesh.ply at 1024 (xatlas 0.415), 0.387 -> 0.637 on a 200k-face Truck at 4096 (xatlas 0.614). Tests: footprint non-overlap, bounded/unbounded pages, rotation; atlas winding and texel-level non-overlap; the segment-quality ratchet tracks coverage. atlasbench gets --pack-footprints and builds on Windows. Python: pack_footprints, unwrap(pack_footprints=True). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every selected face is split at its edge midpoints into four, and every neighbour sharing a split edge into two or three (red-green closure), so the mesh stays conforming; midpoint colors are interpolated, authored normals and face attributes dropped. Moved from openMVS's mesh refinement, which calls it every scale (its CUDA refine output is byte-identical through the move). Python: subdivide_faces. CHANGELOG for 0.5.0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Moved from openMVS's texturing, which fills its atlas pages with it so a mipmapped lookup near a patch averages that patch's surroundings instead of the background; Dilate only fills a band. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each face gets floor(area*density) points plus one more with the probability of the fraction left, uniform in the triangle (Turk), colored from the texture when asked; deterministic per seed. Moved from openMVS, which samples meshes with it for view selection and --sample-mesh (same positions for the same seed). Python: sample_points. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Moved from openMVS and rebuilt: - IsWatertight checks every edge is shared by exactly two faces winding it opposite ways (closed, edge-manifold, consistently oriented) in one O(F) bucket pass over the directed edges, read-only; openMVS only looked for boundary vertices and built adjacency caches to do it. - ComputeVolume spans the tetrahedra with the bounding-box center and sums in double over fixed blocks: exact far from the origin, thread-count independent. ComputeVolume(plane) measures the prisms down to a plane, split into the parts above and below it (fill and cut of an open surface). - Join keeps an attribute only when both meshes carry it, concatenates textures, and appends a live half-edge structure without a rebuild. - SubMesh carries every attribute and only the textures the faces use, with a hashed renumbering for small selections of large meshes. Python: is_watertight, compute_volume, sub_mesh, Mesh.join. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ials LoadOBJ / SaveOBJ, and .obj through Load / Save. The file streams in 32 MB blocks (the next read while the current one parses), each parsed in parallel over newline-aligned chunks in two passes (count, place, parse) so every element lands in its final slot and relative indices resolve against exact global counts. Floats parse correctly rounded (fast_float, new header-only private dependency) and are written in their shortest round-trip form, so save -> load is bit-exact. Every index form, line continuations, xyzrgb colors, polygons split facing their Newell normal (quads by the valid, then shorter, diagonal; larger ones ear-clipped), MTL Kd / map_Kd with options; a material without a loadable map keeps its Kd as a one-texel texture. Save writes one vt per distinct UV of a vertex and one usemtl group per blob. Against openMVS's previous codec on a 26 M-face mesh: save 29.8 s -> 0.82 s, load 30.2 s -> 0.99 s; it also printed six decimals and could not read back its own untextured output. Python: Mesh.load / Mesh.save accept .obj. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cdcseacave
left a comment
cdcseacave
left a comment
There was a problem hiding this comment.
Summary
footprint-packing (7 commits ahead of origin/develop) adds footprint atlas packing, OBJ I/O, volume and watertightness queries, join/submesh, push-pull fill, subdivision, and area-uniform sampling. The new mesh queries, OBJ round-trip, and subdivision are covered by tests and look consistent with their stated formulas. The defects are in footprint placement: a block grid that can be narrower than the page it claims to pack, and a row cursor that skips legal slots, including every rotated placement of a square footprint. Mesh::Join on itself is undefined, and mixed OBJ vertex normals can become NaN.
Issue counts by severity
- bugs: 4
- suggestions: 0
- nits: 0
| continue; // this width cannot make a smaller page | ||
| std::vector<Placed> page; | ||
| std::vector<uint32_t> leftover; | ||
| PackPage(footprints, remaining, width / block, maxHeight / block, params.allowRotation, page, leftover); |
There was a problem hiding this comment.
[bug] The page is accepted in pixels (mask.cols <= maxPageSize) but packed on a grid of width / block by maxHeight / block columns and rows. When the page side is not a multiple of block, a mask that fills the page needs ceil(page / block) blocks and the grid has only floor(page / block), so the mask is left unpacked. PackChartFootprints sets blockSize to page / 2048 and maxPageSize to page (src/AtlasPacking.cpp:575), then PackAtlas asserts every chart packed (src/AtlasPacking.cpp:1101). A 4096-texel atlas whose largest footprint grows to an odd side (4097 with block size 2) hits this: the chart fits the page in pixels and is reported as not fitting. In a release build the assert is gone and those charts are placed from a default rect.
Suggestion: Size the block grid with ceil(page / block), or round the page side up to a multiple of block before dividing.
There was a problem hiding this comment.
Fixed in fcdfee4. The grid now covers the page (ceil(page / block) columns and rows), and a mask may start at block column x only while x*block + width <= page (same for rows), so the partial last block takes only masks that end inside the page. Tests: a 5x5 mask on a 5x5 page with block 4, a 4097x24 mask on a 4097 page with block 2 (PackAtlas's grid), and two 2-texel columns sharing a 6-texel page while two 3-texel ones do not.
| if (p.cols > W) | ||
| continue; | ||
| const int r0 = p.rowOrder.front(), len0 = p.end[r0] - p.start[r0]; | ||
| int& cursor = cursors[(static_cast<uint64_t>(p.cols) << 32) | static_cast<uint32_t>(rows)]; |
There was a problem hiding this comment.
[bug] The search cursor is keyed only by the footprint's block bounding size, and it is written at the end of each orientation (cursor = y at line 171) before the other orientation runs. Two different silhouettes that share a bounding size therefore share a cursor: a shape that does not fit in the low rows advances the cursor, and the next shape of that size never tries those rows. A square footprint shares one key across both orientations, so the rotated profile is never searched: if the unrotated profile finds a row, rotation starts on that row and the y < bestY test skips it; if the unrotated profile finds nothing, the cursor is already at the bottom. On a fixed page, PackChartFootprints turns any such leftover into a failed pack (return 0), so a scale that can hold the charts is rejected.
Suggestion: Key the cursor by the span profile, or drop it, and search both orientations before recording a row as exhausted.
There was a problem hiding this comment.
Fixed in fcdfee4, differently from the suggestion. Keying the cursor by exact span profile was correct but made packing 3x slower (mesh.ply at 1024: 1.34 s -> 4.0 s, same coverage), since almost every chart has its own profile. The per-size cursor stays as a start row; a footprint that finds no place from the cursors now searches the rows they skipped, in both orientations, before it is left over. So a footprint is left over only if the page has no place for it. The rescans of leftovers are what cost time, and they happen in the failing probes of the fit-to-resolution search. That search now uses detail::FootprintsFitOnePage, which stops each page width at its first leftover. Net: mesh.ply 1.34 s -> 1.12 s, 200k-face Truck at 4096 13.4 s -> 13.0 s, coverage unchanged (50.2%, 63.7%). Tests: your left/right halves case (2 pages), and an L plus a square mask whose only place is rotated.
| else | ||
| array.insert(array.end(), otherArray.begin(), otherArray.end()); | ||
| }; | ||
| vertices.insert(vertices.end(), other.vertices.begin(), other.vertices.end()); |
There was a problem hiding this comment.
[bug] Mesh::Join appends other's vertices, attribute arrays, and faces into *this with no separate copy. Join(*this) (and Mesh.join of the same Python object) inserts a vector into itself and range-for's faces while pushing onto it. Both are undefined if reallocation invalidates the source iterators.
Suggestion: Copy other first when &other == this.
There was a problem hiding this comment.
Fixed in fcdfee4: Join(*this) joins a copy. Tested in C++ (live half-edges and textures, ValidateHalfMesh) and Python (mesh.join(mesh)).
| FOREACH (c, cornerVN) { | ||
| const VIndex v = faces[c / 3][c % 3]; | ||
| if (mixed[v]) | ||
| vertexNormals[v] += normals[cornerVN[c]].normalized(); |
There was a problem hiding this comment.
[bug] When a vertex's corners disagree on vn, each corner normal is .normalized() and the sum is .normalize()'d again. A zero-length vn, or corner normals that cancel, has length 0; Eigen's normalize() divides by that and writes NaN into vertexNormals. A single shared vn is stored unchanged, so the NaN only appears on the mixed path.
Suggestion: Skip zero-length normals, and leave the vertex normal at zero when the sum has no length.
There was a problem hiding this comment.
No change needed: Eigen 3.3+ normalize() / normalized() leave a zero vector unchanged (if (z > 0) /= sqrt(z); vcpkg ships 3.4), so a zero vn adds nothing and a cancelling sum stays zero, as suggested. Added MeshIoObj.MixedVertexNormalsStayFinite to pin it: a single zero vn, +z/-z cancelling, +z/-z/zero, and 3z+zero -> unit z.
The texture writers set their flags concurrently, and std::vector<bool> packs neighbouring flags into one word (TSan: data race in _Bit_reference). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cdcseacave
left a comment
cdcseacave
left a comment
There was a problem hiding this comment.
Follow-up posting of the branch review originally performed at 4c85b3c, checked against current head b34066421368b2e70e631698bf317c1eec668b60.
Five findings remain. Two additional findings are posted inline with this review:
- [P1] Oversized block sizes cause an access violation. A 2x2 mask with
blockSize = 2^31reproduced an access violation in Release. Python accepts this value because its validation rejects only zero. - [P2] OBJ filenames containing
#lose textures on round-trip. Saving and loadingrace#1.objreproduced the loss of all textures because the loader truncates the generated material-library reference at#.
The following findings already have review comments; these links avoid duplicate inline reports:
- [P1] Valid atlas dimensions can produce zero pages. Existing comment. With resolution 4097, padding 2, and a chart measuring 4093x20 texels, Release returned zero pages. Standalone packing also rejected a 5x5 mask on a 5x5 page with block size 4.
- [P2] Search cursors incorrectly carry across different footprints. Existing comment. Four 4x4 masks, two left halves followed by two right halves, produced three pages although two suffice.
- [P2] Self-joining invokes undefined behavior. Existing comment.
mesh.Join(mesh)and Pythonmesh.join(mesh)insert source ranges from the destination vectors themselves and append while traversing aliased ranges.
Resolved since the original review: the parallel OBJ texture-export data race from std::vector<bool> was fixed in b340664 by using one byte per texture.
Validation at the originally reviewed head: all 56 targeted C++ tests passed; the edge-case reproductions above exposed defects outside that coverage. Python tests were not run. No source files were changed during the review or this posting.
| std::vector<FootprintPlacement>& placements) | ||
| { | ||
| ASSERT(params.blockSize > 0 && params.maxPageSize >= 0 && params.sizeMultiple >= 0); | ||
| const int block = static_cast<int>(params.blockSize); |
There was a problem hiding this comment.
[P1] Oversized block sizes cause an access violation. blockSize is unsigned, but this conversion can produce a negative block. With a non-empty 2x2 mask and blockSize = 2^31, profile dimensions become zero and ComputeProfile indexes empty span arrays. A Release reproduction exited with an access violation (0xC0000005). The Python binding also accepts this value because it checks only block_size == 0, so a Python call can terminate the interpreter instead of raising an exception.
There was a problem hiding this comment.
Fixed in fcdfee4: FootprintPackParams::maxBlockSize (65536). Outside 1..65536, PackFootprints asserts and packs nothing, and pack_footprints raises ValueError (tested with block_size=2**31). The block area is also computed in 64 bits now.
| } | ||
| out << "# halfmesh " << Version() << ": " << vertices.size() << " vertices, " << faces.size() << " faces\n"; | ||
| if (textured) | ||
| out << "mtllib " << stem << ".mtl\n"; |
There was a problem hiding this comment.
[P2] OBJ filenames containing # lose textures on round-trip. This emits the filename directly into mtllib, and map_Kd likewise emits texture filenames directly. The loader calls StripComment before resolving either statement, so a valid output path such as race#1.obj writes mtllib race#1.mtl but is reloaded as mtllib race. Saving and reloading that path reproduced a successful geometry load with zero textures, although the material library and texture files were written successfully.
There was a problem hiding this comment.
Fixed in fcdfee4 on the loader side, so files from other writers benefit too. In mtllib, usemtl, newmtl and map_Kd, a # inside a name is part of it, and a comment starts only at a # after a blank. Other statements keep the plain comment rule. Tests: a race#1.obj save/load round trip keeps its texture, and lib#3.mtl / mat#4 / map#2.png with trailing # comments load.
…BJ names with '#' - PackFootprints: the block grid covers the page (ceil), a partial last block column/row taking only masks that end inside the page, so a mask as wide as a page that is not a whole number of blocks packs (a 4097-texel atlas with 2-texel blocks returned zero pages). - The per-bounding-size search cursor stays (it is what keeps packing fast), but a footprint that finds no place from the cursors now searches the rows they skipped: it is left over only if the page has no place for it (left/right halves of one size, the rotated place of a square footprint). - PackAtlas's fit-to-resolution probe asks only whether the charts fit one page (detail::FootprintsFitOnePage) and stops at the first leftover: mesh.ply at 1024 packs in 1.12 s instead of 1.34 s, a 200k-face Truck at 4096 in 13.0 s instead of 13.4 s, coverage unchanged (50.2%, 63.7%). - blockSize is bounded by FootprintPackParams::maxBlockSize (65536); outside 1..65536 nothing is packed, and Python raises ValueError (2^31 used to crash). - Mesh::Join(*this) joins a copy instead of appending to the arrays it reads. - OBJ/MTL: in mtllib, usemtl, newmtl and map_Kd a '#' inside a name is part of it and a comment starts after a blank, so race#1.obj keeps its textures on a round trip. - Tests for each, plus mixed OBJ vertex normals that are zero or cancel (Eigen's normalize leaves a zero vector unchanged: no NaN).
…reshold tests - DevelopableMerge counts heap pops (AtlasSegmentStats, now also for the initial merge). The mesh.ply stats test asserts pops <= pushes in every merge; the pre-0.4.1 re-queue fails it (49,955 pops for 15,034 pushes in the initial merge). - CloseHoles: the outline classifier flood-fills only the components that hold a fill candidate, instead of labelling the whole mesh, and no thread pool starts when there is nothing to fill. 800k-face closed sphere plus one isolated triangle: 32 ms -> 2.3 ms (1.4 ms without the triangle); new perf-harness guard. - Tests on both sides of the 3/4 outline share: a 16-sided washer filled at inner radius 0.65 and kept open at 0.66, a tetrahedron missing its base filled at height 1 and kept open at 0.2, at unit scale and scaled 1000x far from the origin. The hole regressions now check ValidateHalfMesh(). The shallow-shell case is documented with the washer. - CHANGELOG: link definitions for 0.4.1, 0.4.2 and 0.5.0.
New CMake option HALFMESH_TEST_HOOKS, on automatically with HALFMESH_BUILD_TESTS / _PERF / _BENCH and passed as a PUBLIC define. A shipped library (vcpkg port, pip wheel) is built without it, so it carries neither the code nor the runtime cost of: - HalfMesh::BuildCount / FFacesCount and their resets: a shared atomic increment on every half-edge build and face harvest. The validation rebuild calls the uncounted BuildImpl / FFacesImpl directly. - Mesh::RemeshStats and RemeshIsotropic's stats argument: 12 clock reads and the count accumulation per iteration. - detail::AtlasSegmentStats: the null checks in the merge heap loop, the per-round stats struct, the split counter. The unused refoldedPairsCollateral and its per-round unordered_set are removed. - Mesh::ValidateHalfMesh, Mesh::ValidateVertexFaces, TriangleBVH::Height / MaxLeafSize, PriorityQueue::IsFaultless, detail::ComputeSegmentationSeeds, detail::CarveFailureRegionForTest. The 3- and 4-argument detail::ChartFacesFold overloads, used only by tests, become default arguments of the one the library calls, and the debug-only `blocked` counter moves under HM_ATLAS_DEBUG. Outputs are unchanged: unwrap and remesh of mesh.ply are byte-identical with the hooks on and off, and the suite passes (642/642).
The flag was kept up to date on every vertex-representative write (SetVHalfedge, saved and restored by the FAdd undo mark), could only be trusted when true, and was read only by GuaranteeAlwaysEven() (no library caller) and the test-only ValidateHalfMesh(). The parity is now computed on demand by one O(V) scan; GuaranteeAlwaysEven() scans before it rebuilds. SetVHalfedge is gone: representatives are written directly. Outputs are unchanged (unwrap and remesh of mesh.ply byte-identical); 642/642 tests pass.
…esh, OBJ I/O (0.5.0) (#11)
Summary
Release 0.5.0: mesh-only algorithms move out of openMVS into halfmesh, and each is rebuilt to be general, fast and exact. openMVS (
improvemntsbranch) now calls all of them throughMVS/MeshHalfMesh.cpp.PackFootprints;PackAtlaspacks chart footprints by defaultMesh::SubdivideFaces(selective 1-to-4, red-green closure)PushPullFill(Util/Raster.h)Mesh::SamplePoints(area-uniform, seeded, texture colors)IsWatertight,ComputeVolume(closed and against a plane),Join,SubMeshLoadOBJ/SaveOBJ)Highlights
mesh.plyat 1024 (xatlas 0.415), and 0.387 → 0.637 on a 200k-face Truck at 4096 (xatlas 0.614).Kd/map_Kd.pack_footprints,subdivide_faces,sample_points,is_watertight,compute_volume,sub_meshandMesh.join..objworks inMesh.load/Mesh.save, andunwrapgainspack_footprints.Tests
Tests.exe0, 1 and 2 pass with 0 ASSERTs on top of this branch.Once this is merged,
v0.5.0gets tagged on develop, and openMVS's port SHA512 is set from that tag.🤖 Generated with Claude Code