claude/bake-ao-single-hierarchy-27AwS - #103
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2af5e5c191
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: befefd27ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df9e50587f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a44e2ae45
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e015e71b5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1e4f903 to
8dde2fe
Compare
Bake AO across all active MeshRenderer descendants of a selected root GameObject as one batch, without requiring a LODGroup. Apply writes only the configured vertex AO channel — other mesh data is untouched. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
Rebuilding the MeshEntry list on every IMGUI pass allocates a fresh renderer array and new MeshEntry per descendant, causing GC churn on larger hierarchies. Cache by selection + mode and only rebuild when they change. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
- Add per-renderer include checkbox list (reuses MeshEntry.include). - Add "Rebake Selected" button: rebakes one hierarchy mesh with current UI settings, merges into existing bakedRaw/FaceArea dictionaries so other meshes' results are preserved. - Add per-FBX overwrite picker after Apply: lists each unique source FBX referenced by the hierarchy, user checks which to rewrite. - Add LightmapTransferTool.ExportVertexColorsToFbx(path, entries) overload that bypasses ResolveFbxPath and walks the supplied entry list — enables per-FBX batching from the Vertex AO tool. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
- Restore BuildLodBatches call in ExecuteBake; prior refactor dropped the batches variable and broke compilation (CS0103). - Preserve per-renderer include flags across RefreshHierarchyEntries by snapshotting them by renderer instance ID. Previously, any selection change or refresh reset user exclusions to true. - Drop unconditional RefreshHierarchyEntries calls in ExecuteBake / LoadFromMesh — OnDrawSidebar's IfNeeded path keeps the list fresh within the same OnGUI pass and won't clobber user edits. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
A hierarchy with empty MeshFilters (vertexCount == 0) tripped ComputeBuffer construction in the GPU path (count must be > 0), causing the whole GPU bake to fail and fall back to CPU. - Filter empty meshes at hierarchy collection time. - Defensive filter of zero-vertex targets/occluders in the GPU Prepare step to protect LODGroup / standalone paths too. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
Clicking a mesh label in the hierarchy list pings the renderer in the Hierarchy window. Clicking an FBX label in the overwrite picker pings the asset in the Project window. Checkbox behaviour is unchanged. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
The UV2 sidecar postprocessor (Uv2AssetPostprocessor.OnPostprocessModel) has a dedicated early-return for FBX overwrite flows — toggled by adding the path to fbxOverwritePaths — but ExportVertexColorsToFbxCore never registered the path. When persistent mode was on and an _uv2data.asset sidecar existed (common after prior UV2 transfer), the postprocessor rebuilt the mesh from stale sidecar data after ExportObjects, nuking any fresh AO written into UV channels (e.g. UV2 X visible as "not saved" after reimport). Register sourceFbxPath in fbxOverwritePaths right before ExportObjects so the postprocessor consumes the flag on the triggered reimport and logs "Skipped sidecar … (FBX overwrite — UV2 already in file)". https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
CopyVertexDataToClone stopped at the first matching cloneMf per scene entry. When an FBX contains multiple MeshFilters that share a single mesh sub-asset (common in hierarchy-mode instancing), every scene entry matched the SAME cloneMf. Repeated scene iterations overwrote the same clone while the other sibling cloneMfs kept the original un-modified sub-asset, so only one renderer ended up with AO after reimport. Invert the loop: index scene entries by mesh name once, then walk every cloneMf and apply its matching entry's colors/UV. Each cloneMf is visited exactly once, and instanced FBX parts all receive the AO data. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
- Invalidate hierarchyEntries cache on EditorApplication.hierarchyChanged. Adding/removing/reparenting children under the selected root now prompts a refresh on the next OnGUI pass; previously stale renderers could be baked or fresh ones missed until the user re-selected the root. (Codex P2) - Match scene entries to cloneMf by FBX sub-asset InstanceID first, falling back to mesh name. Prevents two distinct sub-assets that coincidentally share a name from being conflated during FBX overwrite, and also tolerates working-copy entries whose mesh identity has diverged from the FBX sub-asset. (Codex P2) https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
Two symptoms in hierarchy mode: 1) Colliders found only when sitting as direct children of the selected root. MeshHygieneUtility.FindCollisionObjects scans two levels deep (legacy LODGroup convention) so deeper nests (Market/Props/_COL_*) were invisible to AO. 2) Range test relied on a combined-bbox check plus per-target pivot points. Candidates close to an individual target bbox but far from the combined centroid could fall through. Effective radius was also inflated by the encapsulated bounds, so spread-out target sets produced absurd radii that pulled in unrelated geometry while still missing nearby siblings of a single target. Fixes, all local to VertexAOTool: - New CollectCollisionDescendants recurses the full subtree using strict IsCollisionNodeName (suffix). Wired into CollectLiveCollisionOccluders. - ComputeTargetAnchors also returns per-target world bounds. - IsWithinOccluderRange adds a per-target bbox-vs-candidate-bbox check between the combined and pivot tests, so any single target bbox passing the radius accepts the candidate. - Effective radius now scales the largest single-target extent rather than the encapsulated extent, keeping the neighbourhood tight and proportional to real mesh size. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
Log the CopyVertexDataToClone decision tree so we can see whether the clone mesh actually received UV data, or why it was skipped. Counts: cloneMf visited, matched, actual UV writes, total updates. Also surface warnings when UV copy is skipped because of vertex count mismatch or missing scene UV data — both are silent today and would let 'Vertex data (N updates)' report zero UV writes with a colors-only fallback. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
Fix "FBX overwrite drops AO": ApplyToMesh writes AO into the native buffer of the in-memory FBX sub-asset Mesh. ExportVertexColorsToFbxCore Phase 1 can call SaveAndReimport to fix up importer settings (weld/compression/ generateSecondaryUV/isReadable). That reimport resets the native buffer from disk, wiping the UV/colors the user just applied. Diagnostic was showing visited=1, matched=1, uv2Writes=0 without warning — because GetUVs(2) returned an empty list on the reimported mesh even though the managed reference was still valid. Snapshot colors32/colors and the target UV channel per scene-entry at the start of ExportVertexColorsToFbxCore (keyed by sub-asset name, which is stable across reimport). CopyVertexDataToClone now sources from the snapshot instead of re-reading the live scene mesh, so Phase 1 reimport no longer loses data. Expand Nearby occluder search to the whole scene root in Hierarchy Mode. Previously the search was rooted at the user-selected GameObject — a single-mesh selection had no siblings so nearby=0 even with sparse geometry around in the level. Walk transform.root instead; target/alternate-LOD filtering plus the per-target bbox range test keep the candidate set to actually-nearby geometry. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
When "Use collision meshes as occluders" is on, pre-walk collider names to collect their stripped group keys (UvToolContext. ExtractGroupKey strips _COL/_LOD suffixes). Renderers whose group key matches a collider are skipped — the collider takes its place in the BVH as a low-poly stand-in. Renderers without a matching collider still contribute, so coverage is not lost. Avoids doubling BVH geometry and the self-occlusion artifacts from stacking detailed renderer + low-poly collider over the same object. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
Match rule: a collider node whose name ends with "_COL" (case- insensitive) replaces the renderer with the same prefix. No more ExtractGroupKey (which also strips _LOD and _Hull variants) — the rule is exact and predictable, matching the user's convention. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
FBX convention used by the user puts the "_COL" suffix on the mesh asset (MeshFilter.sharedMesh.name or MeshCollider.sharedMesh.name), not on the GameObject name. Previous detection walked node names via IsCollisionNodeName and so found nothing in these projects. New CollectCollisionMeshCandidates iterates MeshColliders first, then MeshFilters, yielding (GO, mesh) pairs where mesh.name ends with "_COL". Deduped per (GO, mesh). CollectLiveCollisionOccluders and the covered-keys pre-walk both consume it; renderer skip compares against mesh.name (not renderer.name). Drops unused TryGetCollisionMesh helper. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
CollectCollisionMeshCandidates already dedups per (GO, mesh), so the secondary seen-set keyed by mesh.GetInstanceID() was dropping instanced colliders — 10 GOs pointing the same _COL sub-asset at different world positions would collapse to 1 occluder and the other 9 placements would be missing from the BVH. Remove the mesh-ID filter; per-GO iteration preserves each placement with its own transform. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
Toggle width 16 left no room for the IMGUI checkbox widget + margin: the label rendered right on top of the checkbox and the first letter was partially hidden. Widen toggle column to 22 and add a 4px gap before the clickable label. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
SyncFbxOverwriteMap previously collected paths from every entry under hierarchyRoot, so the FBX picker showed unrelated FBX files that the user had unchecked in the Meshes list (or never planned to bake). Filter by e.include and re-sync each paint so toggling Meshes checkboxes refreshes the FBX list immediately. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
Hierarchy-mode "Overwrite Selected FBX" is meant to update vertex channels on existing FBX sub-assets without restructuring. The shared NormalizeExportHierarchy step is destructive in this path: - it walks every MeshFilter under tempRoot and bakes non-identity transforms into mesh vertices, - in hierarchy export only the matched cloneMfs hold cloned meshes; unmatched cloneMfs still reference the live FBX sub-assets, - so BakeTransformIntoMesh mutates the SHARED asset's vertices, visibly displacing every scene MeshFilter using it AND freezing one instance's transform into the exported FBX (broken on next reimport). Add a normalizeHierarchy flag to ExportVertexColorsToFbxCore. The zero-arg LOD path keeps normalization (true). The hierarchy overload passes false — preserves FBX structure, leaves shared asset meshes untouched, only the AO data we wrote into clones is exported. Reported by user: baking AO on a single ceiling mesh in Basement_01.fbx (which has 82 sub-meshes referenced by many scene renderers) corrupted the surrounding furniture transforms. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6a9905a7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
NormalizeExportHierarchy collected every direct child matching _LOD<N>$ into one flat list and renamed them all to <rootName>_LOD<i>. Hierarchies with two or more LOD chains under one root (e.g. Ruins3th_Floor with both Props_Ruins_LOD0..2 and SovietBuilding_BaseAtlas_LOD0..2 children) were collapsed into a single Ruins3th_Floor_LOD0..5 chain — group prefixes lost and unrelated meshes merged into one LOD sequence after re-import. Switch the regex to capture the per-child prefix and bucket children by that prefix, so each LOD chain is normalised independently and its distinct prefix is retained. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
The Overwrite FBX (Vertex Colors) entry point called ExportVertexColorsToFbxCore with normalizeHierarchy: true, but the confirmation dialog promises "Only vertex colors will be updated. UV2 and mesh topology stay unchanged." Normalization renames LOD children and bakes node transforms into mesh vertices — both violate that promise and broke users who relied on stable mesh names across runs. Pass false instead. The two other call sites (hierarchy-mode export and variant pipeline) already pass false. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5bc09e007
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- LightmapTransferTool.ExportVertexColorsToFbxCore: remove the overwrite marker by targetFbxPath, not sourceFbxPath. The marker is added with targetFbxPath, and variant export writes to a different output, so a failure would leave the wrong path in fbxOverwritePaths and skip sidecar UV2 application on the next normal import of that target. - VariantExportPipeline.ExportVariants: drop the StartAssetEditing/StopAssetEditing wrapper around the batch loop. Each ExportSingleVariant calls AssetDatabase.ImportAsset and ModelImporter.SaveAndReimport, both of which are deferred while asset editing is paused, so BuildPrefabClone could load the fresh output FBX before its sub-meshes existed and fail with "contains no meshes". Single trailing Refresh keeps the project view in sync. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
…109) * Add MeshLabArtifactValidator and document external MeshLab settings Detects three classes of artifacts left by the desktop MeshLab tool when processing FBX before Unity import: - Collapsed UV seams (Merge Close / Remove Duplicate Vertices) - Recalculated normals (UseExistingNormals OFF on FBX export) - Broken or wiped UV1 parameterization (Flat Plane filter) Exposed via Tools/Mesh Lab/Validators/Check Imported MeshLab Artifacts. Operates on selected GameObject, Mesh asset, or imported FBX. Read-only; logs through UvtLog. EXPERIMENTS.md gets a new section listing the safe MeshLab filter and export settings, mapped to the symptoms the validator emits. * Preserve mesh.name in variant FBX export pipeline Object.Instantiate(resultMesh) produced clones named "Original(Clone)" which Unity FBX Exporter writes with empty/auto-generated mesh node names; the FBX SDK then defaults the FbxMesh node to the scene name. On reimport every mesh asset shows up as "Scene", erasing per-LOD and per-renderer identifiers needed by sidecar lookup, scene relinking and the new MeshLabArtifactValidator report. Mirrors the existing fix at CopyVertexDataToClone (in-place overwrite path): assign mesh.name = ResolveExportMeshName(...) before handing the mesh to ExportObjects. --------- Co-authored-by: Claude <noreply@anthropic.com>
Source-of-truth document for FBX authoring + tooling. Each rule maps to a patch_fbx_materials report category or a Unity import warning, with notes on UnityMeshLab / TS_ plugin / desktop MeshLab compatibility.
Source-of-truth document for FBX authoring + tooling. Each rule maps to a patch_fbx_materials report category or a Unity import warning, with notes on UnityMeshLab / TS_ plugin / desktop MeshLab compatibility. Co-authored-by: Claude <noreply@anthropic.com>
New FbxExportIntent flags enum + ExportIsolatedChannelsToFbx public entry on LightmapTransferTool. Caller declares which per-vertex channels (UV0-UV7, VertexColors, Normals, Tangents) the re-save is allowed to write; everything else — node names, hierarchy, transforms, materials, untouched per-vertex channels — is inherited from the source FBX via clone-and-snapshot. Mirrors the discipline of ExportVertexColorsToFbxCore (importer prep, .meta backup/restore, Uv2 postprocessor coordination, scene relink, working-copy restore) but never runs NormalizeExportHierarchy or material trim. Existing ExportVertexColorsToFbx* and ExportFbx paths untouched.
New §12 — every FBX-write path goes through the single isolated-
export core; per-tool intent recipe; hard rules ("rework, not
parallel"). Sets the contract that follow-up consolidation commits
implement.
ExportFbxIsolatedCore (renamed from ExportIsolatedChannelsToFbxCore) is now the only FBX-write path. ExportVertexColorsToFbxCore is a ~10-line shim that computes FbxExportIntent from its legacy "AO target UV channel" arg and delegates to the core. The 3 public vcolor wrappers (ExportVertexColorsToFbx*) keep their signatures — external callers (VariantExportPipeline / VertexColorBakingTool / UvPackHierarchyTool) are unaffected. Hierarchy / Materials gating moved inside the core (gated on intent bits), removing the parallel snapshot/copy machinery (VertexDataSnapshot + CopyVertexDataToClone, ~280 LoC) that duplicated the new IsolatedExportSnapshot path. The narrow vcolor intent (VertexColors + maybe one UV) preserves prior behavior; the core can now also handle Hierarchy + Materials when the upcoming ExportFbx LOD-pipeline refactor wires them in. Net -219 LoC. No public signature changes.
ExportFbxIsolatedCore now writes to <target>.fbx.tmp first, verifies the file is non-empty, then atomic File.Replace into the source path (with a temp .fbx.bak rollback window). On exporter failure or zero-byte output, the original FBX on disk is untouched and the tmp file is reaped. Prior behavior overwrote the source directly, leaving a corrupt FBX + stale .meta when the exporter mid-faulted. Pre-export preflight scans the cloned hierarchy for FBX-pipeline- checklist violations: empty/invalid node names (§5.5/§8), generic mesh names like Scene/Geometry (§5.5), placeholder materials Lit/Default/null (§1.5), vertex colors outside [0,1] when intent overwrites VertexColors (§4.2), negative-determinant accumulated scale (§7.8). All findings are logged via UvtLog.Warn and never block the export — the atomic write provides the rollback safety, preflight provides observability. Both apply to every caller automatically (vcolor shim, isolated- channel public API, future LOD-pipeline refactor) — there is no parallel path to maintain.
ExportFbx(bool) is now a back-compat shim onto ExportFbx(bool, FbxExportIntent) defaulting to FbxExportIntent.All — every existing call site (UI buttons, ExportFbxPublic, UvToolHub keyboard shortcut, PrefabBuilderTool) keeps current behavior. When intent is narrow (no Hierarchy and no LodGroup bits) ExportFbx groups entries by source FBX path and delegates each group to ExportFbxIsolatedCore — atomic write, preflight, no Normalize- ExportHierarchy / collision injection / material trim. New tool callers can now pass `FbxExportIntent.UV2`, `VertexColors`, etc. through ExportFbxPublic and get isolation guarantees end-to-end. Wide intent (Hierarchy or LodGroup set) continues through the unchanged LOD-rebuild pipeline. Migrating that path to atomic write + folding mesh-name preservation into the snapshot logic is queued as a follow-up — out of scope for this commit to keep the diff reviewable. The wide path is not "another system"; it's the Hierarchy-bit branch of the same intent-driven entry point.
Brings in the FbxExportIntent API + atomic FBX overwrite + isolated- channel export core (5 code commits on top of the docs-only #110 that was already squash-merged). Required before migrating PrefabBuilderTool.ExecBuildSave to the narrow intent path. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F # Conflicts: # Documentation~/FBX_PIPELINE_CHECKLIST.md
PrefabBuilderTool.ExecBuildSave called LightmapTransferTool.ExportFbxPublic without an intent, which defaulted to FbxExportIntent.All — the wide LOD-rebuild path that mutates hierarchy, materials and the LODGroup component on every save, even when the build session only renamed a node or generated LODs. - Add buildIntent bitfield on the tool, accumulated from the three Build pipeline mutations (ApplyPendingNames -> Hierarchy; NormalizeHierarchy -> Hierarchy | LodGroup | Normals | Tangents because NormalizeChildScales rewrites vertex positions; ExecBuild- GenerateLods -> Hierarchy | LodGroup | AnyUv | VertexColors | Normals | Tangents because new sub-meshes carry fresh channels). - Reset on OnRefresh and after each save. - Fall back to FbxExportIntent.All when nothing was tracked, since Save can be hit after edits performed by other tools (UV2 transfer, vertex color baking) that PrefabBuilder doesn't observe. Also extend FbxExportIntent enum with Hierarchy / Materials / Collision / LodGroup flags. ExportFbx already references them in its narrow-vs- wide dispatch (LightmapTransferTool.cs:2238 and 1911-1931); without the additions in the enum the merged code did not compile. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b953f1534
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var intent = FbxExportIntent.VertexColors; | ||
| int aoUvIdx = ResolveLegacyAoUvChannel(uvChannelOverride); | ||
| if (aoUvIdx >= 0 && aoUvIdx <= 7) | ||
| intent |= (FbxExportIntent)(1 << aoUvIdx); |
There was a problem hiding this comment.
ExportVertexColorsToFbxCore always seeds the export intent with FbxExportIntent.VertexColors, then optionally adds a UV channel. That means callers that only want UV1 updates (for example UvPackHierarchyTool passing uvChannelOverride: 1) still rewrite vertex colors on the FBX. In projects where scene mesh colors differ from the source FBX, this silently clobbers authored vertex colors despite the UI/message saying only UV1 is updated.
Useful? React with 👍 / 👎.
| if (intent.TouchesPerVertex()) | ||
| { | ||
| if (srcImporter.weldVertices) | ||
| { srcImporter.weldVertices = false; needsReimport = true; } | ||
| if (srcImporter.meshCompression != ModelImporterMeshCompression.Off) | ||
| { srcImporter.meshCompression = ModelImporterMeshCompression.Off; needsReimport = true; } | ||
| if (srcImporter.meshOptimizationFlags != 0) | ||
| { srcImporter.meshOptimizationFlags = 0; needsReimport = true; } |
There was a problem hiding this comment.
This block disables weldVertices, meshCompression, and meshOptimizationFlags for any per-vertex intent, including VertexColors-only exports. Phase 5 only restores isReadable, so a plain vertex-color overwrite now leaves those importer settings permanently changed. That is a regression in behavior for color-only export and can unexpectedly increase vertex count/memory or alter downstream import characteristics after one export action.
Useful? React with 👍 / 👎.
- UvPackHierarchyTool: stop routing UV1-only pack through ExportVertexColorsToFbx. The vcolor wrapper unconditionally seeds FbxExportIntent.VertexColors, so the UV1 atlas pack also rewrote authored vertex colors in the source FBX. Switch to the explicit ExportIsolatedChannelsToFbx(path, entries, FbxExportIntent.UV1) entry point so only UV1 is touched. - LightmapTransferTool.ExportFbxIsolatedCore: restore weldVertices, meshCompression, meshOptimizationFlags and generateSecondaryUV in Phase 5 if Phase 1 disabled them. Previously only isReadable was restored, so a single per-vertex isolated re-save (e.g. vertex color overwrite) permanently flipped these importer toggles on the user's source FBX, silently changing downstream import behaviour after one export click. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
ModelImporter.meshOptimizationFlags is typed MeshOptimizationFlags (enum) — newer Unity versions reject the implicit two-way conversion to/from int. Type the snapshot variable as the enum so the snapshot and restore assignments compile cleanly. https://claude.ai/code/session_01Cm3FDtRJqnEyDT2qaJUfkT
When a [MenuItem] action shares its path with a child submenu (e.g.
"Tools/Mesh Lab" alongside "Tools/Mesh Lab/Validators/..."), Unity
treats the parent path as a submenu only and silently drops the
parent action. The Open Mesh Lab entry was therefore invisible —
hovering Tools/Mesh Lab showed nothing but the Validators submenu.
Move the window opener under the same namespace ("Tools/Mesh Lab/
Open Mesh Lab") so the parent stays a submenu and both entries are
visible.
https://claude.ai/code/session_01Cm3FDtRJqnEyDT2qaJUfkT
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
UnityMeshLab/Editor/Tools/PrefabBuilderTool.cs
Line 1931 in 68fb1cd
Record the LODGroup before merge rewrites LODs
When merging same-material renderers, this SetLODs call changes the LODGroup renderer arrays but the undo group only records the first MeshFilter and the destroyed GameObjects. If the user undoes the merge, the deleted renderers can come back while the LODGroup still points at the merged renderer list, leaving restored objects absent from LOD switching. Record ctx.LodGroup before mutating the LOD array, and persist prefab-instance modifications when applicable.
UnityMeshLab/Editor/Tools/PrefabBuilderTool.cs
Line 1400 in 68fb1cd
Persist LOD moves on prefab instances
Moving a renderer between LOD slots mutates the LODGroup array directly after Undo.RecordObject, but this path bypasses LodGroupUtility.ApplyLods and never calls PrefabUtility.RecordPrefabInstancePropertyModifications. On a prefab instance, the move can appear to work in the current scene but be lost when the scene reloads or the prefab is reapplied. Route this through the shared LOD utility or record prefab-instance modifications after SetLODs.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| RestoreScene(); | ||
| DestroyPackedMeshes(); |
There was a problem hiding this comment.
When the selected root is just a container, ExecutePack changes Selection.activeGameObject to the first packed child so the canvas refreshes; UvToolHub.OnSelectionChange then calls ActiveTool.OnRefresh, and this refresh immediately restores/destroys all packed meshes. In that normal hierarchy-mode flow, packedMeshes is cleared before the user can Apply or Overwrite, so Pack UV1 appears to do nothing. Gate this cleanup when the new selection is still under hierarchyRoot, or avoid destructive cleanup for the tool’s own selection change.
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
|
|
||
| string sourceFbxPath = ctx?.SourceFbxPath; |
There was a problem hiding this comment.
In Hierarchy Mode, the tool builds hierarchyEntries from the selected root even when the shared context has no current FBX path for that container selection. Bake & Export All then uses only ctx.SourceFbxPath, so selecting a prefab/container root either fails with “No source FBX path” or can reuse a stale path from a previous selection instead of the FBX represented by the included hierarchy entries. Resolve the source path from the included entries in hierarchy mode, and reject or group multi-FBX selections explicitly.
Useful? React with 👍 / 👎.
| Uv2AssetPostprocessor.bypassPaths.Add(sourceFbxPath); | ||
| srcImporter.SaveAndReimport(); |
There was a problem hiding this comment.
For FBXs whose importer settings change vertex count or ordering, this SaveAndReimport runs after the snapshots were already captured from the previous imported meshes. The clone loaded after importer prep can then have a different vertex count, so CopyIsolatedSnapshotsToClone skips the mesh as a mismatch and isolated UV/vertex-color exports report no matching updates. Rebuild the per-mesh snapshots after the importer settings are locked, or avoid reimporting between snapshot and clone.
Useful? React with 👍 / 👎.
| if (restoreGenerateSecondaryUV) | ||
| { srcImporter.generateSecondaryUV = true; needsRestoreReimport = true; } |
There was a problem hiding this comment.
When the original importer had Generate Lightmap UVs enabled and the isolated export writes Unity UV channel 1, restoring generateSecondaryUV and immediately reimporting causes Unity to regenerate that channel and replace the UV data that was just written to the FBX. The export file may contain the packed UVs, but the imported mesh users see right after the tool runs is clobbered. Keep this setting disabled for channel-1 exports, or reapply the exported data after the final restore import.
Useful? React with 👍 / 👎.
| if (restoreGenerateSecondaryUV) | ||
| { srcImporter.generateSecondaryUV = true; needsRestoreReimport = true; } |
There was a problem hiding this comment.
When the original importer had Generate Lightmap UVs enabled and the isolated export writes Unity UV channel 1, restoring generateSecondaryUV and immediately reimporting causes Unity to regenerate that channel and replace the UV data that was just written to the FBX. The export file may contain the packed UVs, but the imported mesh users see right after the tool runs is clobbered. Keep this setting disabled for channel-1 exports, or reapply the exported data after the final restore import.
Useful? React with 👍 / 👎.
| var positions = mesh.vertices; | ||
| var normals = mesh.normals; | ||
| var triangles = mesh.triangles; |
There was a problem hiding this comment.
When the selected imported mesh has Read/Write disabled, Validate reads the CPU vertex and triangle buffers before checking readability, which Unity throws for common FBX imports. Running Tools/Mesh Lab/Validators/Check Imported MeshLab Artifacts on those assets now aborts instead of producing a report, so check mesh.isReadable before accessing these arrays or return a non-readable warning.
Useful? React with 👍 / 👎.
| out string outFbxPath, out string outPrefabPath, out string err)) | ||
| return Fail(variant.suffix, variant.color, err); | ||
|
|
||
| int painted = BakeSolidColorOnEntries(entries, variant.color); |
There was a problem hiding this comment.
Batch variant export paints the shared source meshes in-place before each variant export and never restores their original vertex colors. After Bake & Export All, the scene/imported meshes are left with the last variant’s solid color, and later exports can start from that contaminated state; snapshot and restore colors/colors32 around the batch or apply colors only to temporary export clones.
Useful? React with 👍 / 👎.
Seven contained fixes from the latest Codex pass: - LightmapTransferTool isolated core: stop restoring generateSecondaryUV in Phase 5. It is only ever disabled when the intent writes UV1, so re-enabling + reimporting regenerated channel 1 and clobbered the UV1 just authored into the FBX. Authored UV1 must win, so the setting now stays off. (Regression from the prior importer-restore fix.) - MeshLabArtifactValidator: guard mesh.isReadable before touching CPU buffers (vertices/normals/triangles/GetUVs). Non-readable imported meshes threw and aborted the whole validation menu; now reported as a "not readable" warning. - PrefabBuilderTool merge: Undo.RecordObject(LodGroup) before rewriting its renderer arrays and RecordPrefabInstancePropertyModifications after SetLODs. Undo previously restored the merged GameObjects while the LODGroup still pointed at the merged list, dropping them from LOD switching. - PrefabBuilderTool move-between-LODs: persist the LOD-array change on prefab instances via RecordPrefabInstancePropertyModifications so the move survives scene reload / prefab reapply. - UvPackHierarchyTool.OnRefresh: skip the destructive packed-mesh cleanup when the new selection is still under hierarchyRoot. ExecutePack re-selects a packed child to refresh the canvas, and that selection change was wiping packed results before the user could Apply/Overwrite. - VertexColorBakingTool variant export: in hierarchy mode resolve the source FBX from the included entries (ctx.SourceFbxPath can be empty or stale for container selections) and reject multi-FBX selections. - VariantExportPipeline: snapshot and restore vertex colors around the batch so shared source/working meshes aren't left wearing the last variant's solid color. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
ExportFbxIsolatedCore captured per-mesh snapshots from the tool's working mesh (built from the current import) but then disabled weldVertices / meshCompression / meshOptimizationFlags and reimported the source FBX before cloning it. weldVertices is ON by default, so for typical imports the reimported clone had a different vertex count than the snapshot, and CopyIsolatedSnapshotsToClone silently skipped every mesh on the mismatch — isolated UV1 / vertex-color exports reported "no matching updates" and wrote nothing. Remove the weld/compression/optimization pre-export reimport from the isolated path: - The snapshot source (e.g. the packed UV1 clone in UvPackHierarchyTool, or the AO-baked working copy) shares its vertex layout with the CURRENT import; the clone is also loaded from the current import, so they match when we don't reimport. - The wide LOD-rebuild path (ExportFbx) never did this reimport and works, so it's the correct precedent. - Those settings were restored right after export anyway, so the final re-imported FBX kept the user's original weld/optimization state regardless. The pre-export reimport therefore never affected the exported result — it only desynced the clone from the snapshot. isReadable handling stays (layout-safe) and generateSecondaryUV stays disabled for UV1 intents (so the authored UV1 isn't regenerated). The remaining, rare case where an FBX still re-imports at a different vertex count (Generate Lightmap UVs splitting vertices for a UV1 export) now logs an actionable warning instead of a bare mismatch line. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
Consolidates the 3 bake-ao commits not yet in the prefab-builder branch (bb8df3b Codex-review batch, 68fb1cd menu submenu, dab24f8 CS0266 fix) into the #111 rework so the prefab-builder branch carries all of #103. All files auto-merged except Editor/Tools/PrefabBuilderTool.cs, which the rework rewrote. Resolved by keeping the rewrite and re-porting bb8df3b's Undo/prefab-safety fixes into the surviving FixMerge path: - Undo.RecordObject(ctx.LodGroup, "Merge") before the renderer-array rewrite (so undo restores the LODGroup's renderer list, not just the GameObjects). - RecordPrefabInstancePropertyModifications after SetLODs (so the merge persists on prefab instances across scene reload / reapply). bb8df3b's move-between-LODs hunk has no target — that path was removed in the rework ("reorder removed"), so nothing to port there. UNVERIFIED: no Unity compile in this environment. Staging branch for the #111 consolidation — compile in Unity before finalizing.
Resolve conflicts between the PrefabBuilder / FbxExportIntent line and main's transfer-pipeline / UvProgress work (1.0.3–1.0.8): - LightmapTransferTool.cs: keep HEAD's meshName resolution at the loop top; add main's TangentValidator.EnforceTangentsMatchOriginal call (dropped main's duplicate meshName re-declaration). - LodGenerationTool.cs: keep HEAD's refactor that delegates to LodPipelineOps.Generate (which PrefabBuilder also uses). Main's inline additions (CompactLodArray, MeshEntry registration, ClearAllCaches) already live in LodPipelineOps. - LodPipelineOps.cs: port main's non-modal cancelable UvProgress into Generate (replacing the modal EditorUtility.DisplayProgressBar) so main's progress UX isn't lost by taking HEAD's LOD-gen side. - VertexColorBakingTool.cs: keep HEAD's BuildSettingsFromUI() + color- space log; fold main's new binaryHit field into the helper (it was missing there, so the toggle was silently ignored on HEAD). - CHANGELOG.md / EXPERIMENTS.md: union both sides' entries. https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F
2 participants
Bake AO across all active MeshRenderer descendants of a selected root
GameObject as one batch, without requiring a LODGroup. Apply writes
only the configured vertex AO channel — other mesh data is untouched.
https://claude.ai/code/session_01CkRooSFA5QW3Kox9AAFe3F