refactor: reduce duplication - #309
Merged
Merged
Conversation
Extract shared rotation-quaternion helper and consolidate primitive UI show/hide logic into a single configurator to cut copy/paste blocks. Made-with: Cursor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates quaternion construction into TransformMath; centralizes primitive UI setup; introduces Viewport/App settings keys and FSAA support with render-window teardown/rebuild and camera-rig save/restore; stages media at build time; improves Assimp normal-map/tangent handling and refines RTShader normal-map linking. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PreferencesUI
participant QSettings
participant MainWindow
participant OgreWidget
participant SpaceCamera
User->>PreferencesUI: select new FSAA value
PreferencesUI->>QSettings: write Viewport/fsaaSamples
PreferencesUI->>MainWindow: notify setting changed
MainWindow->>MainWindow: rebuildAllOgreViewports()
MainWindow->>OgreWidget: rebuildRenderWindow()
OgreWidget->>SpaceCamera: getViewportPose()
OgreWidget->>OgreWidget: teardownOgreWindow()
OgreWidget->>OgreWidget: initOgreWindow(with FSAA from QSettings)
OgreWidget->>SpaceCamera: applyViewportPose()
OgreWidget->>MainWindow: return (render window rebuilt)
MainWindow->>User: render windows refreshed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Ensure dev builds have materials + RTSS (OgreUnifiedShader.h) available beside the executable so viewports render correctly. Made-with: Cursor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/TransformMath.h (1)
8-19: Optional: document precedence when multiple components are non-zero.The comment states "only one axis is expected to be non-zero at a time" but the implementation silently picks X, then Y, then Z (short-circuit order) if a caller ever violates that precondition. Since this is a shared utility header now, a brief note about that fallback order (or an assertion) would prevent surprises if a future caller passes a multi-axis vector expecting combined rotation.
📝 Proposed doc tweak
// Builds a quaternion matching the editor's rotate-vector convention: // only one axis is expected to be non-zero at a time (X->UNIT_Y, Y->UNIT_Z, Z->UNIT_X). +// If multiple components are non-zero, precedence is X, then Y, then Z; the others are ignored. inline Ogre::Quaternion buildRotationQuat(const Ogre::Vector3 &rotate)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TransformMath.h` around lines 8 - 19, The comment notes buildRotationQuat silently prefers rotate.x then rotate.y then rotate.z when multiple components are non-zero; update the function's documentation (above buildRotationQuat) to explicitly state that precedence (X first, then Y, then Z) and that only one axis is intended, or add a runtime check/assertion inside buildRotationQuat that detects multiple non-zero components of the rotate parameter and either logs/asserts or handles it explicitly; reference the buildRotationQuat function and the rotate parameter when adding the doc note or assertion so callers know the fallback order.src/CMakeLists.txt (1)
357-365: Placement is correct; fullmedia/re-copy optimization not applicable with current CMake.The
$<TARGET_FILE_DIR:${CMAKE_PROJECT_NAME}>resolves toContents/MacOS/for macOS bundles and the runtime directory on Linux/Windows, matching the search paths inRTShaderHelper.cpp(appDir + "/media/RTShaderLib") andMaterialEditorQML.cpp(media/...,../media/...).Using
copy_directoryunconditionally recopies all files on every (re)link. The optimizationcopy_directory_if_differentwould avoid this, but it requires CMake ≥ 3.26. The project currently requires 3.24.0, so adopting it would require bumping the minimum version first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CMakeLists.txt` around lines 357 - 365, The current add_custom_command block uses ${CMAKE_PROJECT_NAME} and copy_directory which unconditionally recopies media/ on every link; to avoid changing behavior on unsupported CMake versions, keep the placement but make the copy conditional: detect CMake version (compare CMAKE_VERSION or CMAKE_MINOR/MINOR) and, if >= 3.26, call copy_directory_if_different, otherwise fall back to the existing copy_directory call; reference the existing add_custom_command, ${CMAKE_PROJECT_NAME}, copy_directory and the alternative copy_directory_if_different and ensure any version bump is handled by adjusting CMAKE_MINIMUM_REQUIRED before switching to the new API.src/PrimitivesWidget.h (1)
38-44: Consider a config struct over a 16-parameter signature.Eight
boolflags interleaved withQStringoverrides are easy to miscount at call sites (e.g., "was radius2 before or after height?"). A small aggregate with designated initializers at the call site would make intent self-documenting while keeping the helper's body identical.♻️ Proposed refactor
- void applyPrimitiveUiConfig(const QString &type, - bool sizeX, bool sizeY, bool sizeZ, - bool radius, bool radius2, bool height, - const QString &radiusText, const QString &radius2Text, - bool segX, bool segY, bool segZ, - const QString &segXText, const QString &segYText, const QString &segZText, - bool uvTileVisible, bool switchUvVisible = true); + struct PrimitiveUiConfig { + QString type; + bool sizeX = false, sizeY = false, sizeZ = false; + bool radius = false, radius2 = false, height = false; + QString radiusText, radius2Text; + bool segX = false, segY = false, segZ = false; + QString segXText, segYText, segZText; + bool uvTileVisible = false; + bool switchUvVisible = true; + }; + void applyPrimitiveUiConfig(const PrimitiveUiConfig &cfg);Call sites then read like
applyPrimitiveUiConfig({ .type = tr("Cube"), .sizeX = true, .sizeY = true, .sizeZ = true, .segX = true, ... });.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PrimitivesWidget.h` around lines 38 - 44, Replace the long applyPrimitiveUiConfig signature with a single config struct: define a PrimitiveUiConfig (or similar) containing fields for type, sizeX/sizeY/sizeZ, radius/radius2/height, radiusText/radius2Text, segX/segY/segZ and their text fields, and uvTileVisible/switchUvVisible with sensible defaults; change the applyPrimitiveUiConfig declaration to accept const PrimitiveUiConfig& config and update the implementation to read from config (no behavior change), then update all call sites to construct the struct with designated initializers (e.g., PrimitiveUiConfig{ .type=..., .sizeX=true, ... }) so intent is explicit and parameter order mistakes are avoided.src/PrimitivesWidget.cpp (1)
293-332: MigratesetUiRoundedBoxtoapplyPrimitiveUiConfigto finish the deduplication.This method is structurally identical to the already-refactored ones (it's the same show/hide + label-override pattern) and is the largest remaining duplicate block the PR set out to remove.
♻️ Proposed refactor
void PrimitivesWidget::setUiRoundedBox() { - edit_type->setText(tr("Rounded Box")); - - gb_Geometry->show(); - gb_Mesh->show(); - - edit_sizeX->show(); - edit_sizeY->show(); - edit_sizeZ->show(); - - label_sizeX->show(); - label_sizeY->show(); - label_sizeZ->show(); - - label_radius->show(); - label_radius2->hide(); - label_height->hide(); - - edit_radius->show(); - edit_radius2->hide(); - edit_height->hide(); - - label_radius->setText(tr("Chamfer")); - - label_numSegX->show(); - label_numSegY->show(); - label_numSegZ->show(); - - edit_numSegX->show(); - edit_numSegY->show(); - edit_numSegZ->show(); - - setUVTileVisible(true); - - label_numSegX->setText(tr("Seg X")); - label_numSegY->setText(tr("Seg Y")); - label_numSegZ->setText(tr("Seg Z")); - + applyPrimitiveUiConfig(tr("Rounded Box"), + true, true, true, + true, false, false, + tr("Chamfer"), QString(), + true, true, true, + tr("Seg X"), tr("Seg Y"), tr("Seg Z"), + true); }
setUiSpringis correctly left alone — it only showsgb_Meshand would not fit the helper without further generalization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PrimitivesWidget.cpp` around lines 293 - 332, The setUiRoundedBox implementation duplicates the same show/hide and label-override pattern used by other primitives; refactor it to call the shared helper applyPrimitiveUiConfig instead of duplicating UI logic. Replace the body of setUiRoundedBox so it invokes applyPrimitiveUiConfig with the appropriate parameters or flags to show gb_Geometry/gb_Mesh, show edit_sizeX/Y/Z, edit_radius (hide edit_radius2/edit_height), set label_radius text to "Chamfer", show label_numSegX/Y/Z and edit_numSegX/Y/Z, call setUVTileVisible(true), and set the Seg X/Y/Z label texts; ensure you use the same property names (gb_Geometry, gb_Mesh, edit_sizeX/edit_sizeY/edit_sizeZ, label_radius, edit_radius, edit_radius2, edit_height, label_numSegX/Y/Z, edit_numSegX/Y/Z, setUVTileVisible) so the new call matches other usages of applyPrimitiveUiConfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/CMakeLists.txt`:
- Around line 357-365: The current add_custom_command block uses
${CMAKE_PROJECT_NAME} and copy_directory which unconditionally recopies media/
on every link; to avoid changing behavior on unsupported CMake versions, keep
the placement but make the copy conditional: detect CMake version (compare
CMAKE_VERSION or CMAKE_MINOR/MINOR) and, if >= 3.26, call
copy_directory_if_different, otherwise fall back to the existing copy_directory
call; reference the existing add_custom_command, ${CMAKE_PROJECT_NAME},
copy_directory and the alternative copy_directory_if_different and ensure any
version bump is handled by adjusting CMAKE_MINIMUM_REQUIRED before switching to
the new API.
In `@src/PrimitivesWidget.cpp`:
- Around line 293-332: The setUiRoundedBox implementation duplicates the same
show/hide and label-override pattern used by other primitives; refactor it to
call the shared helper applyPrimitiveUiConfig instead of duplicating UI logic.
Replace the body of setUiRoundedBox so it invokes applyPrimitiveUiConfig with
the appropriate parameters or flags to show gb_Geometry/gb_Mesh, show
edit_sizeX/Y/Z, edit_radius (hide edit_radius2/edit_height), set label_radius
text to "Chamfer", show label_numSegX/Y/Z and edit_numSegX/Y/Z, call
setUVTileVisible(true), and set the Seg X/Y/Z label texts; ensure you use the
same property names (gb_Geometry, gb_Mesh, edit_sizeX/edit_sizeY/edit_sizeZ,
label_radius, edit_radius, edit_radius2, edit_height, label_numSegX/Y/Z,
edit_numSegX/Y/Z, setUVTileVisible) so the new call matches other usages of
applyPrimitiveUiConfig.
In `@src/PrimitivesWidget.h`:
- Around line 38-44: Replace the long applyPrimitiveUiConfig signature with a
single config struct: define a PrimitiveUiConfig (or similar) containing fields
for type, sizeX/sizeY/sizeZ, radius/radius2/height, radiusText/radius2Text,
segX/segY/segZ and their text fields, and uvTileVisible/switchUvVisible with
sensible defaults; change the applyPrimitiveUiConfig declaration to accept const
PrimitiveUiConfig& config and update the implementation to read from config (no
behavior change), then update all call sites to construct the struct with
designated initializers (e.g., PrimitiveUiConfig{ .type=..., .sizeX=true, ... })
so intent is explicit and parameter order mistakes are avoided.
In `@src/TransformMath.h`:
- Around line 8-19: The comment notes buildRotationQuat silently prefers
rotate.x then rotate.y then rotate.z when multiple components are non-zero;
update the function's documentation (above buildRotationQuat) to explicitly
state that precedence (X first, then Y, then Z) and that only one axis is
intended, or add a runtime check/assertion inside buildRotationQuat that detects
multiple non-zero components of the rotate parameter and either logs/asserts or
handles it explicitly; reference the buildRotationQuat function and the rotate
parameter when adding the doc note or assertion so callers know the fallback
order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48cd4279-5b8f-45a3-a067-6194cb72e4c5
📒 Files selected for processing (6)
src/CMakeLists.txtsrc/MeshTransform.cppsrc/PrimitivesWidget.cppsrc/PrimitivesWidget.hsrc/SkeletonTransform.cppsrc/TransformMath.h
- RTShaderHelper: stop adding duplicate SGX_PerPixelLighting; the scheme global render state already provides it and a second instance broke lighting/normal-map generation. - MeshImporterExporter: run applyNormalMapsToEntity after Assimp import to build tangents when missing and refresh RTSS on live materials. - FFPLib_Texturing: include RTSLib_Colour.glsl so ENABLE_LINEAR_COLOUR is defined for generated shaders. - resources.cfg.in / resources_d.cfg.in: register materials/programs paths (keep General in sync for Debug vs Release). - plugins_d.cfg.in: load Codec_STBI like release for consistent textures. Made-with: Cursor
- Preferences > Viewport: Off / 2x / 4x / 8x (default 4), stored as Viewport/fsaaSamples - OgreWidget: pass FSAA to createRenderWindow; rebuildRenderWindow on change - SpaceCamera: save/restore viewport pose across rebuild - MainWindow: rebuildAllOgreViewports; apply clip/speed from settings after init Made-with: Cursor
- Add ViewportSettingsKeys for shared key literals (C++/tests) - Extract tryApplyViewportSetting to reduce setSetting complexity - Guard mViewport in rebuildRenderWindow before reading visibility mask - Tests: MSAA, rebuild, pose round-trip, preference persistence; fsaaSamples() Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/CMakeLists.txt (1)
358-364: Prefer project-local source path for media copy input.Line 363 uses
${CMAKE_SOURCE_DIR}, which points to the top-level CMake project and can break if this project is embedded as a subdirectory. Prefer${PROJECT_SOURCE_DIR}(or${CMAKE_CURRENT_SOURCE_DIR}/..) for robustness.Suggested CMake diff
add_custom_command(TARGET ${CMAKE_PROJECT_NAME} POST_BUILD COMMAND ${CMAKE_COMMAND} -E copy_directory - ${CMAKE_SOURCE_DIR}/media + ${PROJECT_SOURCE_DIR}/media $<TARGET_FILE_DIR:${CMAKE_PROJECT_NAME}>/media COMMENT "Copying media/ next to QtMeshEditor (materials + RTSS)" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CMakeLists.txt` around lines 358 - 364, The add_custom_command uses ${CMAKE_SOURCE_DIR} as the media copy source which breaks when the project is embedded; update the command to use a project-local source variable such as ${PROJECT_SOURCE_DIR} (or ${CMAKE_CURRENT_SOURCE_DIR}/..) instead of ${CMAKE_SOURCE_DIR} so the copy_directory source becomes project-local; modify the add_custom_command invocation that references ${CMAKE_SOURCE_DIR}/media to use ${PROJECT_SOURCE_DIR}/media (or the CMAKE_CURRENT_SOURCE_DIR variant) to make embedding robust.src/OgreWidget.cpp (1)
77-133: Destructor duplicatesteardownOgreWindow(); delegate to the helper.The new
teardownOgreWindow()(Lines 224–264) is an almost line-for-line copy of the destructor's OGRE cleanup block (Lines 82–130) — same ordering (remove frame listener → reset camera → remove viewports → detach render target → deactivate → null out), sametry { ... } catch (...) {}nesting, same two null-out branches. Any future fix (e.g., additional OGRE shutdown step) now has to be applied in two places and will silently drift.Have the destructor reuse the helper:
♻️ Proposed fix
OgreWidget::~OgreWidget() { - // Safely clean up OGRE resources - // Order is important: remove listeners first, then destroy camera, then detach render target, then remove viewports - - if(mOgreRoot) - { - try { - // Remove frame listener first - mOgreRoot->removeFrameListener(this); - } catch (...) { - // Ignore exceptions during shutdown - } - } - - // Destroy camera BEFORE removing viewports (viewport may reference camera) - // This must happen before removing viewports - mCamera.reset(); - - if(mOgreWindow) - { - try { - ... - } catch (...) { - mViewport = nullptr; - mOgreWindow = nullptr; - } - } - + teardownOgreWindow(); destroy(); }Also applies to: 224-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OgreWidget.cpp` around lines 77 - 133, Destructor OgreWidget::~OgreWidget currently duplicates the OGRE cleanup logic that lives in teardownOgreWindow(); remove the duplicated cleanup block from the destructor and simply call teardownOgreWindow() (preserving the existing call to destroy() afterwards), ensuring teardownOgreWindow() performs the same ordered steps (removeFrameListener on mOgreRoot, mCamera.reset(), mOgreWindow->removeAllViewports(), mOgreRoot->detachRenderTarget(mOgreWindow), mOgreWindow->setActive(false), and nulling mViewport/mOgreWindow with the same exception-safety) so there is a single authoritative shutdown path to maintain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qml/PreferencesDialog.qml`:
- Around line 303-307: The Component.onCompleted block currently parses the
persisted "Viewport/fsaaSamples" and only updates the UI variable msaaSelection
when the value is valid, but it leaves invalid values in settings; modify the
Component.onCompleted handler (the code that calls readSetting and sets
msaaSelection) to validate parsed v (acceptable set: 0,2,4,8) and if v is
invalid, reset the persisted setting via the same settings API (e.g.,
writeSetting or setSetting) to a safe default (4) and then set msaaSelection to
that default so both the UI state and QSettings are normalized; reference the
existing readSetting call, msaaSelection, and Component.onCompleted when
locating where to add the write/reset logic.
In `@src/mainwindow.cpp`:
- Around line 1766-1772: The rebuildAllOgreViewports loop lacks a breadcrumb and
a defensive null check for EditorViewport pointers; update
MainWindow::rebuildAllOgreViewports to call
SentryReporter::addBreadcrumb("viewport", "rebuild all ogre viewports") before
iterating, and inside the loop guard against null vp and null OgreWidget by
checking the EditorViewport* (vp) is non-null and that vp->getOgreWidget()
returns a valid pointer before calling rebuildRenderWindow(); ensure breadcrumb
placement occurs once at start of the operation to track the user action.
In `@src/OgreWidget_test.cpp`:
- Around line 230-248: This test mutates global QSettings keys
(ViewportSettingsKeys::fsaaSamples(), cameraSpeed(), nearClip(), farClip()) but
doesn't restore them, causing cross-test leakage; fix by reading and saving the
previous values via QSettings::value(...) before setting the test values and
restore them at the end of the test (or use QScopedValueRollback equivalents) so
that after calling widget->rebuildRenderWindow() and assertions you set the
original values back into QSettings; ensure restoration runs regardless of test
failures (e.g., in a RAII/try-finally style teardown) and reference the same
keys (ViewportSettingsKeys::fsaaSamples/cameraSpeed/nearClip/farClip) when
restoring.
In `@src/SpaceCamera.cpp`:
- Around line 150-162: applyViewportPose currently sets
mTarget->setOrientation(targetOrient) but then calls setCameraPosition(camWorld)
which internally uses mTarget->lookAt(...) and overwrites the orientation
(losing roll); fix by ensuring the captured orientation is restored after
positioning (e.g., call mTarget->setOrientation(targetOrient) again after
setCameraPosition returns) or change setCameraPosition to accept an optional
flag/orientation to skip mTarget->lookAt when a target orientation is supplied
so the roll component is preserved; reference applyViewportPose,
setCameraPosition, mTarget->setOrientation(targetOrient), and mTarget->lookAt.
---
Nitpick comments:
In `@src/CMakeLists.txt`:
- Around line 358-364: The add_custom_command uses ${CMAKE_SOURCE_DIR} as the
media copy source which breaks when the project is embedded; update the command
to use a project-local source variable such as ${PROJECT_SOURCE_DIR} (or
${CMAKE_CURRENT_SOURCE_DIR}/..) instead of ${CMAKE_SOURCE_DIR} so the
copy_directory source becomes project-local; modify the add_custom_command
invocation that references ${CMAKE_SOURCE_DIR}/media to use
${PROJECT_SOURCE_DIR}/media (or the CMAKE_CURRENT_SOURCE_DIR variant) to make
embedding robust.
In `@src/OgreWidget.cpp`:
- Around line 77-133: Destructor OgreWidget::~OgreWidget currently duplicates
the OGRE cleanup logic that lives in teardownOgreWindow(); remove the duplicated
cleanup block from the destructor and simply call teardownOgreWindow()
(preserving the existing call to destroy() afterwards), ensuring
teardownOgreWindow() performs the same ordered steps (removeFrameListener on
mOgreRoot, mCamera.reset(), mOgreWindow->removeAllViewports(),
mOgreRoot->detachRenderTarget(mOgreWindow), mOgreWindow->setActive(false), and
nulling mViewport/mOgreWindow with the same exception-safety) so there is a
single authoritative shutdown path to maintain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e54d125a-7892-46ad-ba48-0cd2d7ca17f6
📒 Files selected for processing (14)
qml/PreferencesDialog.qmlsrc/CMakeLists.txtsrc/OgreWidget.cppsrc/OgreWidget.hsrc/OgreWidget_test.cppsrc/PropertiesPanelController.cppsrc/PropertiesPanelController_test.cppsrc/SpaceCamera.cppsrc/SpaceCamera.hsrc/SpaceCamera_test.cppsrc/ViewportSettingsKeys.hsrc/mainwindow.cppsrc/mainwindow.hsrc/mainwindow_test.cpp
✅ Files skipped from review due to trivial changes (1)
- src/ViewportSettingsKeys.h
Comment on lines
+303
to
+307
| Component.onCompleted: { | ||
| var v = parseInt(readSetting("Viewport/fsaaSamples", 4)) | ||
| if (v === 0 || v === 2 || v === 4 || v === 8) | ||
| msaaSelection = v | ||
| } |
There was a problem hiding this comment.
Normalize invalid persisted MSAA values, not just UI state.
Right now, unsupported stored values only fall back in the local UI selection. Consider also correcting persisted settings to avoid stale invalid config remaining in QSettings.
Suggested QML diff
Component.onCompleted: {
var v = parseInt(readSetting("Viewport/fsaaSamples", 4))
- if (v === 0 || v === 2 || v === 4 || v === 8)
- msaaSelection = v
+ if (v === 0 || v === 2 || v === 4 || v === 8) {
+ msaaSelection = v
+ } else {
+ msaaSelection = 4
+ writeSetting("Viewport/fsaaSamples", 4)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Component.onCompleted: { | |
| var v = parseInt(readSetting("Viewport/fsaaSamples", 4)) | |
| if (v === 0 || v === 2 || v === 4 || v === 8) | |
| msaaSelection = v | |
| } | |
| Component.onCompleted: { | |
| var v = parseInt(readSetting("Viewport/fsaaSamples", 4)) | |
| if (v === 0 || v === 2 || v === 4 || v === 8) { | |
| msaaSelection = v | |
| } else { | |
| msaaSelection = 4 | |
| writeSetting("Viewport/fsaaSamples", 4) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/PreferencesDialog.qml` around lines 303 - 307, The Component.onCompleted
block currently parses the persisted "Viewport/fsaaSamples" and only updates the
UI variable msaaSelection when the value is valid, but it leaves invalid values
in settings; modify the Component.onCompleted handler (the code that calls
readSetting and sets msaaSelection) to validate parsed v (acceptable set:
0,2,4,8) and if v is invalid, reset the persisted setting via the same settings
API (e.g., writeSetting or setSetting) to a safe default (4) and then set
msaaSelection to that default so both the UI state and QSettings are normalized;
reference the existing readSetting call, msaaSelection, and
Component.onCompleted when locating where to add the write/reset logic.
Comment on lines
+1766
to
+1772
| void MainWindow::rebuildAllOgreViewports() | ||
| { | ||
| for (EditorViewport* vp : mDockWidgetList) { | ||
| if (OgreWidget* w = vp->getOgreWidget()) | ||
| w->rebuildRenderWindow(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add breadcrumb (and a null guard) in viewport rebuild loop.
This is a significant operation initiated from preferences; it should be tracked and made slightly more defensive.
Suggested diff
void MainWindow::rebuildAllOgreViewports()
{
+ SentryReporter::addBreadcrumb("ui.action", "Rebuild all Ogre viewports");
for (EditorViewport* vp : mDockWidgetList) {
+ if (!vp) continue;
if (OgreWidget* w = vp->getOgreWidget())
w->rebuildRenderWindow();
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void MainWindow::rebuildAllOgreViewports() | |
| { | |
| for (EditorViewport* vp : mDockWidgetList) { | |
| if (OgreWidget* w = vp->getOgreWidget()) | |
| w->rebuildRenderWindow(); | |
| } | |
| } | |
| void MainWindow::rebuildAllOgreViewports() | |
| { | |
| SentryReporter::addBreadcrumb("ui.action", "Rebuild all Ogre viewports"); | |
| for (EditorViewport* vp : mDockWidgetList) { | |
| if (!vp) continue; | |
| if (OgreWidget* w = vp->getOgreWidget()) | |
| w->rebuildRenderWindow(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mainwindow.cpp` around lines 1766 - 1772, The rebuildAllOgreViewports
loop lacks a breadcrumb and a defensive null check for EditorViewport pointers;
update MainWindow::rebuildAllOgreViewports to call
SentryReporter::addBreadcrumb("viewport", "rebuild all ogre viewports") before
iterating, and inside the loop guard against null vp and null OgreWidget by
checking the EditorViewport* (vp) is non-null and that vp->getOgreWidget()
returns a valid pointer before calling rebuildRenderWindow(); ensure breadcrumb
placement occurs once at start of the operation to track the user action.
Comment on lines
+230
to
+248
| QSettings settings; | ||
| settings.setValue(ViewportSettingsKeys::fsaaSamples(), 2); | ||
| settings.setValue(ViewportSettingsKeys::cameraSpeed(), 1.5); | ||
| settings.setValue(ViewportSettingsKeys::nearClip(), 0.05); | ||
| settings.setValue(ViewportSettingsKeys::farClip(), 5000.0); | ||
|
|
||
| const QColor bg(18, 52, 86); | ||
| widget->setBackgroundColor(bg); | ||
|
|
||
| EXPECT_NO_THROW(widget->rebuildRenderWindow()); | ||
| app->processEvents(); | ||
|
|
||
| EXPECT_EQ(widget->getBackgroundColor(), bg); | ||
| ASSERT_NE(widget->getSpaceCamera(), nullptr); | ||
| ASSERT_NE(widget->getSpaceCamera()->getCamera(), nullptr); | ||
| EXPECT_FLOAT_EQ(widget->getSpaceCamera()->getCameraSpeed(), 1.5f); | ||
| EXPECT_DOUBLE_EQ(widget->getSpaceCamera()->getCamera()->getNearClipDistance(), 0.05); | ||
| EXPECT_DOUBLE_EQ(widget->getSpaceCamera()->getCamera()->getFarClipDistance(), 5000.0); | ||
| } |
There was a problem hiding this comment.
Restore modified QSettings values at test end to prevent cross-test leakage.
Line 230 onward mutates shared settings keys but does not restore previous values. This can make later tests order-dependent.
Suggested test-hardening diff
TEST_F(OgreWidgetTest, RebuildRenderWindowPreservesBackgroundAndKeepsCamera)
{
QSettings settings;
+ const QVariant savedFsaa = settings.value(ViewportSettingsKeys::fsaaSamples());
+ const QVariant savedSpeed = settings.value(ViewportSettingsKeys::cameraSpeed());
+ const QVariant savedNear = settings.value(ViewportSettingsKeys::nearClip());
+ const QVariant savedFar = settings.value(ViewportSettingsKeys::farClip());
+
settings.setValue(ViewportSettingsKeys::fsaaSamples(), 2);
settings.setValue(ViewportSettingsKeys::cameraSpeed(), 1.5);
settings.setValue(ViewportSettingsKeys::nearClip(), 0.05);
settings.setValue(ViewportSettingsKeys::farClip(), 5000.0);
@@
EXPECT_FLOAT_EQ(widget->getSpaceCamera()->getCameraSpeed(), 1.5f);
EXPECT_DOUBLE_EQ(widget->getSpaceCamera()->getCamera()->getNearClipDistance(), 0.05);
EXPECT_DOUBLE_EQ(widget->getSpaceCamera()->getCamera()->getFarClipDistance(), 5000.0);
+
+ if (savedFsaa.isValid()) settings.setValue(ViewportSettingsKeys::fsaaSamples(), savedFsaa);
+ else settings.remove(ViewportSettingsKeys::fsaaSamples());
+ if (savedSpeed.isValid()) settings.setValue(ViewportSettingsKeys::cameraSpeed(), savedSpeed);
+ else settings.remove(ViewportSettingsKeys::cameraSpeed());
+ if (savedNear.isValid()) settings.setValue(ViewportSettingsKeys::nearClip(), savedNear);
+ else settings.remove(ViewportSettingsKeys::nearClip());
+ if (savedFar.isValid()) settings.setValue(ViewportSettingsKeys::farClip(), savedFar);
+ else settings.remove(ViewportSettingsKeys::farClip());
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OgreWidget_test.cpp` around lines 230 - 248, This test mutates global
QSettings keys (ViewportSettingsKeys::fsaaSamples(), cameraSpeed(), nearClip(),
farClip()) but doesn't restore them, causing cross-test leakage; fix by reading
and saving the previous values via QSettings::value(...) before setting the test
values and restore them at the end of the test (or use QScopedValueRollback
equivalents) so that after calling widget->rebuildRenderWindow() and assertions
you set the original values back into QSettings; ensure restoration runs
regardless of test failures (e.g., in a RAII/try-finally style teardown) and
reference the same keys
(ViewportSettingsKeys::fsaaSamples/cameraSpeed/nearClip/farClip) when restoring.
Comment on lines
+150
to
+162
| void SpaceCamera::applyViewportPose(const Ogre::Vector3& targetWorld, const Ogre::Vector3& camWorld, | ||
| const Ogre::Quaternion& targetOrient) | ||
| { | ||
| if (!mTarget || !mCameraNode || !mCamera) | ||
| return; | ||
| Ogre::SceneNode* parent = mTarget->getParentSceneNode(); | ||
| if (parent) | ||
| mTarget->setPosition(parent->convertWorldToLocalPosition(targetWorld)); | ||
| else | ||
| mTarget->setPosition(targetWorld); | ||
| mTarget->setOrientation(targetOrient); | ||
| setCameraPosition(camWorld); | ||
| } |
There was a problem hiding this comment.
Camera roll is lost across rebuilds: setOrientation(targetOrient) is clobbered by setCameraPosition.
setCameraPosition(camWorld) (Lines 126–132) internally calls mTarget->lookAt(pos, Ogre::Node::TS_WORLD), which recomputes mTarget's orientation from the direction vector (using world Y as up). As a result, the mTarget->setOrientation(targetOrient) on Line 160 is effectively a no-op, and any roll component in targetOrient (e.g. from the user's roll() interaction) is silently discarded every time the render window is rebuilt (e.g., when the user changes MSAA). The pose round-trip test in src/SpaceCamera_test.cpp likely doesn't exercise a rolled rig, which is why this wasn't caught.
Re-apply the captured orientation after repositioning, or avoid the lookAt call when the orientation is already known.
🐛 Proposed fix: restore orientation after positioning
void SpaceCamera::applyViewportPose(const Ogre::Vector3& targetWorld, const Ogre::Vector3& camWorld,
const Ogre::Quaternion& targetOrient)
{
if (!mTarget || !mCameraNode || !mCamera)
return;
Ogre::SceneNode* parent = mTarget->getParentSceneNode();
if (parent)
mTarget->setPosition(parent->convertWorldToLocalPosition(targetWorld));
else
mTarget->setPosition(targetWorld);
- mTarget->setOrientation(targetOrient);
setCameraPosition(camWorld);
+ // setCameraPosition() uses lookAt() which overwrites the target's orientation
+ // (and drops any roll). Restore it explicitly after repositioning.
+ mTarget->setOrientation(targetOrient);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SpaceCamera.cpp` around lines 150 - 162, applyViewportPose currently sets
mTarget->setOrientation(targetOrient) but then calls setCameraPosition(camWorld)
which internally uses mTarget->lookAt(...) and overwrites the orientation
(losing roll); fix by ensuring the captured orientation is restored after
positioning (e.g., call mTarget->setOrientation(targetOrient) again after
setCameraPosition returns) or change setCameraPosition to accept an optional
flag/orientation to skip mTarget->lookAt when a target orientation is supplied
so the roll component is preserved; reference applyViewportPose,
setCameraPosition, mTarget->setOrientation(targetOrient), and mTarget->lookAt.
QSettings values round through Ogre::Real; exact double equality to 0.05 failed on Linux CI (0.05000000074505806 vs 0.05). Made-with: Cursor
- Add AppSettingsKeys (Sentry/Telemetry/Appearance) to remove duplicated literals - Decompose tryApplyViewportSetting into small helpers; extract theme palette apply - Use AppSettingsKeys in SentryReporter and tests Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/PropertiesPanelController.cpp (1)
863-867: Minor:setPalette(QColor)sets only the Window color role.Using
QApplication::setPalette(QColor("ghostwhite"))creates a palette where only certain roles are derived from that color. For a complete "light" theme, you may want to explicitly set multiple roles (similar to the "dark" branch) to ensure consistent styling across all widget types.The current implementation may work fine for basic widgets but could produce inconsistent results for complex controls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PropertiesPanelController.cpp` around lines 863 - 867, The light-theme branch currently calls QApplication::setPalette(QColor(QStringLiteral("ghostwhite"))) which only affects the Window role; replace this with creating a QPalette and explicitly set the common roles (e.g. Window, Base, Button, Text, ButtonText, WindowText, Highlight, HighlightedText) to appropriate light colors (using QColor/QStringLiteral where needed) and then call QApplication::setPalette(palette) so the themeLower == QStringLiteral("light") branch mirrors the explicit role assignments used in the dark branch (locate this logic around themeLower and QApplication::setPalette in PropertiesPanelController.cpp).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/PropertiesPanelController.cpp`:
- Around line 887-890: The code treats AppSettingsKeys::sentryEnabled() and
AppSettingsKeys::telemetryEnabled() as separate stored keys but both only call
SentryReporter::setEnabled(value.toBool()) when setSetting() is invoked, and
only sentryEnabled is read/persisted; fix by either consolidating to a single
canonical key or by making the two independent: if they are the same concept
remove telemetryEnabled and use only AppSettingsKeys::sentryEnabled() everywhere
(update callers and CLI mapping), otherwise change
PropertiesPanelController::setSetting() to update both persistent storage and
runtime behavior correctly — i.e., when key ==
AppSettingsKeys::telemetryEnabled() update its own stored value and add distinct
runtime handling (or map telemetryEnabled reads to SentryReporter appropriately)
so that SentryReporter::setEnabled() reflects the intended canonical source.
---
Nitpick comments:
In `@src/PropertiesPanelController.cpp`:
- Around line 863-867: The light-theme branch currently calls
QApplication::setPalette(QColor(QStringLiteral("ghostwhite"))) which only
affects the Window role; replace this with creating a QPalette and explicitly
set the common roles (e.g. Window, Base, Button, Text, ButtonText, WindowText,
Highlight, HighlightedText) to appropriate light colors (using
QColor/QStringLiteral where needed) and then call
QApplication::setPalette(palette) so the themeLower == QStringLiteral("light")
branch mirrors the explicit role assignments used in the dark branch (locate
this logic around themeLower and QApplication::setPalette in
PropertiesPanelController.cpp).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24c6d8b2-3503-466d-9797-542c2f33337e
📒 Files selected for processing (6)
src/AppSettingsKeys.hsrc/CMakeLists.txtsrc/PropertiesPanelController.cppsrc/PropertiesPanelController_test.cppsrc/SentryReporter.cppsrc/SentryReporter_test.cpp
✅ Files skipped from review due to trivial changes (1)
- src/AppSettingsKeys.h
- Extend Manager valid extensions, import/scene dialogs, welcome dialog - Asset browser + CLI format label; qtmesh scan include/allowed_formats - Tests and MCP tool description Made-with: Cursor
Bump version to 2.29.0. ScanEngine no longer treats AI_SCENE_FLAGS_INCOMPLETE as a hard load failure so animation-only FBX (Mixamo/retarget takes) scan correctly. Match AssimpToOgreImporter policy, set FBX preserve-pivots off, factor isAssimpResultLoadFailure(), and add unit tests for the gate. Made-with: Cursor
|
5 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
src/TransformMath.hand reuse from mesh/skeleton transforms.PrimitivesWidget::applyPrimitiveUiConfig(...).Why
Sonar duplication is above target; this removes a few of the larger repeated blocks and should lower overall duplication percentage.
Test plan
Made with Cursor
Summary by CodeRabbit