quads follow-up: bones/transforms/lighting after n-gon import - #335
Merged
Conversation
Addresses both Codex P1 findings on PR #332 (chunk 4) before they reach master. Issue 1 — bone-handle drift on skinned meshes loadFromAssimpFile stored aiBone mesh-local indices in EditableBoneAssignment::boneIndex. The GUI import path (MeshProcessor) instead resolves aiBone->mName against the loaded Ogre::Skeleton and stores Ogre::Bone::getHandle(). When a topology op re-emitted VertexBoneAssignments via resizeEntityBuffers, those mesh-local indices were re-interpreted as Ogre handles, so vertices rebound to whichever bones happened to occupy those handle slots. Fix: add an optional Ogre::Skeleton* parameter to loadFromAssimpFile. When non-null, aiBone->mName is resolved against it (matching MeshProcessor) and the resulting handle is stored. Bones that don't resolve are skipped so we never emit wild handles. EditModeController passes the live entity's skeleton when entering edit mode. Issue 2 — Z-up overlay rotation on FBX/glTF assets MeshProcessor bakes a +90°-around-X rotation into rendered buffers for assets declared Z-up (FBX UpAxis = 2), so the Ogre scene-graph stays Y-up without a node rotation. loadFromAssimpFile read raw aiMesh vertices unchanged, so on Z-up assets the editable representation lived in pre-bake space while rendered buffers were post-bake — vertex/edge/face overlays appeared rotated 90° relative to the on-screen geometry, and a commit would write the rotated positions back, silently rotating the entity. Fix: add an `isZup` parameter to loadFromAssimpFile. When true, apply the same +90°-around-X bake to position, normal, and tangent before storing them. MeshImporterExporter caches the source up-axis under "qtme.source_up_axis" alongside the existing source-path / convert-LH caches, and EditModeController reads it back when re-entering edit mode. EditableMesh::commitToEntity / resizeEntityBuffers now also erase this cache key when the live buffers diverge from the source. Tests: two new standalone regression tests cover the Z-up bake math and the unskinned-mesh shape of the new bone-skeleton parameter. The skinned-mesh skeleton-lookup case is exercised by EditModeController integration tests at run time.
Addresses the deferred lighting/RTSS regression flagged on PR #334. Root cause After any Edit-Mode topology op (subdivide, extrude, bevel, knife, merge, delete/dissolve, undo/redo, …) bump-mapped meshes loaded through the n-gon import path went dark and lost their normal map. Two compounding issues: 1. EditableMesh::buildSubMeshBuffers checks only `vertices[0] .hasTangent` to decide whether to add a VES_TANGENT element to the rebuilt declaration. New vertices created by the op default- construct EditableVertex (zero-valued Vector4 tangent), so: (a) if the first vertex retained tangents the declaration kept VES_TANGENT but new vertices wrote (0,0,0,0), making RTSS's SRS_NORMALMAP TBN math collapse to zeros; (b) if the asset came in via loadFromAssimpFile (which deliberately omits aiProcess_CalcTangentSpace because that flag forces triangulation), every vertex has hasTangent=false and the declaration drops VES_TANGENT entirely. Either way the bump map was effectively gone. 2. Each topology op had its OWN inline _deinitialise/_initialise + invalidateMaterial block; the undo/redo path (EditMeshTopologyCommand::applyMeshState) had a third copy that skipped the RTSS hook entirely. So a Subdivide-then-Ctrl-Z lost lighting even when the redo would have restored it. Fix Centralise post-topology-op refresh into a new static method EditModeController::rewriteEntityAfterTopologyChange(Entity*). It: a) Detects bump-map intent by scanning every subentity's material for a `normal_map`/`NormalMap` TUS. If any subentity is bump- mapped, force `Mesh::buildTangentVectors` (storeParityInW=true) BEFORE _deinitialise/_initialise. Order matters: calling it AFTER _initialise is too late — the SubEntity already linked against the old (no-tangent) declaration and the RTSS shaders compile against that stale layout. b) Saves per-subentity material overrides before the deinit/init (Ogre resets them to the SubMesh default), restores after. c) Re-runs MeshImporterExporter::applyNormalMapsToEntity so RTSS re-attaches its SRS_NORMALMAP sub-render-state against the fresh tangents. invalidateMaterial alone only drops cached shader programs; the SRS_NORMALMAP gets dropped on removeShaderBasedTechnique inside applyNormalMap, so we must call it again to re-add it. d) Final invalidateMaterial pass to keep behaviour identical to the old per-op blocks for materials that aren't bump-mapped. All five inline copies in EditModeController.cpp (extrude, bevel commit, bevel cancel, knife commit, generic post-op via applyTopologyMutationNoSurvivor) now call this helper, and so does EditMeshTopologyCommand::applyMeshState in TransformCommands.cpp — so undo/redo gets the same treatment. Tests Standalone regression test confirms the helper is reachable as a static method (so TransformCommands.cpp's qualified call survives a refactor that might shove it back into an anonymous namespace) and null-tolerant. Full bump-map / RTSS exercise needs a GL context which the test infra doesn't provide on macOS; coverage there is via hand smoke tests on the bump-mapped Mixamo asset (subdivide / extrude / undo / redo all confirmed visually preserving the bump map and per-pixel lighting).
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 49f5a2b589
ℹ️ 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".
| // doesn't survive a buffer-format change), so we must run it again | ||
| // to guarantee the SRS_NORMALMAP is re-bound against the new | ||
| // tangents. Materials without a normal-map TUS are no-ops here. | ||
| MeshImporterExporter::applyNormalMapsToEntity(ent); |
There was a problem hiding this comment.
rewriteEntityAfterTopologyChange now runs on every topology mutation and undo/redo, but it unconditionally calls MeshImporterExporter::applyNormalMapsToEntity, which performs mat->load() without exception handling. If any sub-entity uses an unloaded or broken material resource, that load can throw and abort the edit operation; this is a regression from the old path that only invalidated RTSS materials and did not force-load them during topology edits.
Useful? React with 👍 / 👎.
Codex P2 on PR #335: applyNormalMapsToEntity is now called from rewriteEntityAfterTopologyChange on every topology op AND every undo/redo, so any single broken/unresolvable material would now abort the entire edit op via an unhandled `mat->load()` throw — a regression from the old path that only invalidated RTSS without forcing a load. Wrap the load call in a try/catch and skip the offending sub-entity on failure. Logs the material name + Ogre exception description so we don't lose the diagnostic.
PR #335's SonarCloud quality gate failed on cognitive complexity: - rewriteEntityAfterTopologyChange: 47 (limit 25) - enterEditMode: 32 (limit 25) plus a deprecation warning on the old buildTangentVectors overload. Refactor (no behaviour change): - File-scope `entityWantsTangents`, `rebuildMeshTangents`, `invalidateEntityRtssMaterials` factor out the three loops inside `rewriteEntityAfterTopologyChange`. Switches to the non-deprecated buildTangentVectors signature. - File-scope `tryLoadEditableMeshNGonPath` factors out the four- nested-try-block n-gon-import attempt from `enterEditMode`. `rewriteEntityAfterTopologyChange` and `enterEditMode` now read top- to-bottom as plain sequences of named steps. All 234 standalone tests still pass.
5 tasks
|
fernandotonon
added a commit
that referenced
this pull request
Apr 29, 2026
PR #335's SonarCloud quality gate failed on cognitive complexity: - rewriteEntityAfterTopologyChange: 47 (limit 25) - enterEditMode: 32 (limit 25) plus a deprecation warning on the old buildTangentVectors overload. Refactor (no behaviour change): - File-scope `entityWantsTangents`, `rebuildMeshTangents`, `invalidateEntityRtssMaterials` factor out the three loops inside `rewriteEntityAfterTopologyChange`. Switches to the non-deprecated buildTangentVectors signature. - File-scope `tryLoadEditableMeshNGonPath` factors out the four- nested-try-block n-gon-import attempt from `enterEditMode`. `rewriteEntityAfterTopologyChange` and `enterEditMode` now read top- to-bottom as plain sequences of named steps. All 234 standalone tests still pass.
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three follow-up fixes for the n-gon import path on
feat/quads. Stacked offfeat/quads; targets it (not master). Closes the three known issues we documented when chunks 4 / 4b / 5a landed.1. Bone-handle drift on skinned meshes (Codex P1 on #332)
loadFromAssimpFilestored aiBone mesh-local indices inEditableBoneAssignment::boneIndex. The GUI import path (MeshProcessor) instead resolvesaiBone->mNameagainst the loadedOgre::Skeletonand storesOgre::Bone::getHandle(). After a topology op re-emittedVertexBoneAssignmentsviaresizeEntityBuffers, those mesh-local indices were re-interpreted as Ogre handles — vertices rebound to whichever bones happened to occupy those handle slots.Fix: new optional
Ogre::Skeleton*parameter onloadFromAssimpFile. When non-null,aiBone->mNameis resolved against it (matchingMeshProcessor) and the resulting handle is stored. Bones that don't resolve are skipped so we never emit wild handles.EditModeControllerpasses the live entity's skeleton when entering edit mode.2. Z-up overlay rotation on FBX/glTF Z-up assets (Codex P1 on #332)
MeshProcessorbakes a +90°-around-X rotation into rendered buffers for assets declared Z-up (FBXUpAxis = 2).loadFromAssimpFileread rawaiMeshvertices unchanged, so the editable representation lived in pre-bake space while rendered buffers were post-bake — vertex/edge/face overlays appeared rotated 90° relative to the on-screen geometry, and a commit would write the rotated positions back, silently rotating the entity.Fix: new
isZupparameter onloadFromAssimpFile. When true, applies the same +90°-around-X bake to position, normal, and tangent before storing.MeshImporterExportercaches the source up-axis underqtme.source_up_axisalongside the existing source-path / convert-LH caches;EditModeControllerreads it back when re-entering edit mode.3. Bump map + per-pixel lighting drop after topology ops (deferred from #334)
After any Edit-Mode topology op on a bump-mapped n-gon-imported asset, the model went dark and lost its normal map. Two compounding issues:
a.
EditableMesh::buildSubMeshBufferschecks onlyvertices[0].hasTangentto decide whether to addVES_TANGENTto the rebuilt declaration — new vertices default-construct with zero-valued tangents, so RTSS's SRS_NORMALMAP TBN math collapsed.b. Each topology op had its own inline
_deinitialise/_initialise + invalidateMaterialblock; the undo/redo path had a third copy that skipped the RTSS hook entirely.Fix: centralise post-op refresh into a new static
EditModeController::rewriteEntityAfterTopologyChange(Entity*)that:normal_map/NormalMapTUS;Mesh::buildTangentVectorsBEFORE_deinitialise/_initialise(so the SubEntity vertex-decl cache reads the corrected layout on_initialise);MeshImporterExporter::applyNormalMapsToEntityso RTSS re-attachesSRS_NORMALMAPagainst the fresh tangents.All five inline copies in
EditModeController.cppnow call this helper.EditMeshTopologyCommand::applyMeshState(undo/redo) calls it too — so a Subdivide-then-Ctrl-Z preserves lighting state.Test plan
Known follow-ups (separate PRs)
feat/quads)