Port mm-viewer and icp-log-viewer from nanogui to Dear ImGui - #79
Open
jlblancoc wants to merge 10 commits into
Open
Port mm-viewer and icp-log-viewer from nanogui to Dear ImGui#79jlblancoc wants to merge 10 commits into
jlblancoc wants to merge 10 commits into
Conversation
Adds mp2p_icp_viz/3rdparty/imgui as a git submodule (docking branch), built once as a private static library (imgui::imgui, never installed) via mp2p_icp_viz/3rdparty/imgui_static, shared by mm-viewer and icp-log-viewer. Also adds a small ImGuiAppShell helper wrapping the GLFW window/context/docking boilerplate common to both apps. First step of the ongoing port from nanogui to Dear ImGui; see ~/plans/mp2p_icp_imgui_port.md.
Rewrites mm-viewer/main.cpp on top of the shared ImGuiAppShell and mrpt::imgui::CImGuiSceneView, replacing the old CDisplayWindowGUI + nanogui sidebar with dockable ImGui panels: "Map viewer" (file load/export + coordinate readout), "View", "Maps", and "Travelling", plus a "3D View" window hosting the scene. Feature set kept at parity with the old app: layer visibility/recolorization, clip/FOV/ortho/2D view options, camera travelling keyframe animation, mouse/camera coordinate readout with map/ENU/lat-lon units, keyboard navigation, and persisted UI/camera state across sessions. Adds a reusable SimpleFileDialog (self-contained ImGui file browser, no native/system dialog dependency) to imgui_app_common for the Open/Export prompts, and a setupDefaultLayout hook on ImGuiAppShell so each app can define its own default docking arrangement on first run (Map viewer docked left, View/Maps/Travelling tabbed below it, 3D View filling the remaining background area) without hardcoding app-specific window names into the shared shell. Part of the ongoing port from nanogui to Dear ImGui; see ~/plans/mp2p_icp_imgui_port.md.
Previously written next to the process's current working directory. Now resolved via the same cfgpath.h helper already used for each app's own settings file, landing at e.g. ~/.config/mm-viewer/mm-viewer.imgui.ini instead of polluting whatever directory the app happens to be launched from.
Re-adds the "Map frame"/"ENU frame" axis-corner mini-viewports (ensureMiniCornerViewports/updateMiniCornerView) that were dropped during the initial ImGui port. mrpt::imgui::CImGuiSceneView only renders the scene's "main" viewport under MRPT 2.x, so these stay invisible for now, but are kept and updated every frame since MRPT 3.x is expected to support rendering named viewports the same way. Noted in agents.md so this isn't mistaken for dead code later. Also splits the default docking layout so "Maps" gets its own docked panel instead of being tabbed with View/Travelling.
Rewrites icp-log-viewer/main.cpp on the same ImGuiAppShell + mrpt::imgui::CImGuiSceneView infrastructure used by mm-viewer, split into dockable panels: Control (record selector, navigation, iteration slider), Summary, Variables, Maps (layer visibility + export), View, Pairings, and Manual (6-DoF pose nudge), plus a 3D View window. Feature set kept at parity with the old app, including autoplay, keyboard navigation, pairing visualization, prior/uncertainty ellipsoids, and persisted UI/camera state. Point-cloud regeneration caching is now driven by comparing mp2p_icp::render_params_t (like mm-viewer) instead of an explicit regenerateMaps bool parameter, forced on record change. Also fixes an ImGui duplicate-ID bug surfaced while testing: the global/local layer checkboxes in the "Maps" panel used the bare layer name as their ID, colliding whenever both maps share a layer name (e.g. "2d_lidar" in the demo dataset) -- scoped with PushID/PopID. Restores the small axis-corner gizmo mini-viewport (kept inert under MRPT 2.x, same rationale as mm-viewer; see agents.md). Part of the ongoing port from nanogui to Dear ImGui; see ~/plans/mp2p_icp_imgui_port.md.
Run via mp2p_icp_core/scripts/formatter.sh; no functional changes.
📝 WalkthroughWalkthroughThe PR migrates ChangesDear ImGui migration
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Viewer
participant ImGuiAppShell
participant CImGuiSceneView
participant ConfigFile
User->>Viewer: launch viewer
Viewer->>ConfigFile: load UI and camera settings
Viewer->>ImGuiAppShell: initialize and run
ImGuiAppShell->>Viewer: invoke renderFrame
Viewer->>CImGuiSceneView: update scene and camera
Viewer->>User: render dockable panels and 3D view
User->>Viewer: change controls or select file
Viewer->>CImGuiSceneView: rebuild visualization
Viewer->>ConfigFile: save UI and camera settings
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (4)
mp2p_icp_viz/apps/mm-viewer/main.cpp (4)
889-904: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
app.layerVisible[lyName]silently insertsfalsefor unseen layers.Using
operator[]in this read path means any layer name not already present (e.g. ifupdateGuiAfterLoadingNewMap()was skipped after a partial load) defaults to hidden rather than visible, which is the opposite of the initialization at Line 638. Prefer an explicit lookup with a visible-by-default fallback.♻️ Proposed change
- if (!app.layerVisible[lyName]) + const auto itV = app.layerVisible.find(lyName); + const bool isVisible = (itV == app.layerVisible.end()) ? true : itV->second; + if (!isVisible) { continue; // hidden }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mp2p_icp_viz/apps/mm-viewer/main.cpp` around lines 889 - 904, Update the visibility check in the layer-rendering loop around app.layerVisible to use an explicit lookup instead of operator[]. Treat missing layer entries as visible by default, while preserving the existing hidden-layer continue behavior for entries explicitly set to false.
259-271: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueQuadratic iteration over keyframes.
Restarting from
begin()andstd::advance()-ing on every iteration makes this O(n²). A single pass over the interpolator is equivalent and simpler.♻️ Proposed refactor
app.camTravellingLabels.clear(); - for (size_t i = 0; i < app.camTravelling.size(); i++) - { - auto it = app.camTravelling.begin(); - std::advance(it, static_cast<std::ptrdiff_t>(i)); - - app.camTravellingLabels.push_back(mrpt::format( - "[%02u] t=%.02fs pose=%s", static_cast<unsigned int>(i), - mrpt::Clock::toDouble(it->first), it->second.asString().c_str())); - } + size_t i = 0; + for (const auto& [t, p] : app.camTravelling) + { + app.camTravellingLabels.push_back(mrpt::format( + "[%02u] t=%.02fs pose=%s", static_cast<unsigned int>(i++), + mrpt::Clock::toDouble(t), p.asString().c_str())); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mp2p_icp_viz/apps/mm-viewer/main.cpp` around lines 259 - 271, Update rebuildCamTravellingLabels to iterate directly over app.camTravelling with a single iterator-based pass, while maintaining a separate incrementing index for the label number; remove the repeated begin() and std::advance() calls so label generation is linear.
92-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePersisted UI state omits some panel fields.
colorIntensityIdx,recolorizeByFieldIdx,mouseUnitsIdx,animFPS, andtravellingInterpIdxare part of the panel state but are not round-tripped throughappCfginmainShowGui()(Lines 1468-1522), unlike the neighbouring booleans/floats. Consider persisting at least the colormap selection for parity with the rest of the View/Maps options.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mp2p_icp_viz/apps/mm-viewer/main.cpp` around lines 92 - 159, Update mainShowGui() appCfg persistence to round-trip the panel fields colorIntensityIdx, recolorizeByFieldIdx, mouseUnitsIdx, animFPS, and travellingInterpIdx, including at minimum colorIntensityIdx. Keep their existing AppState defaults and ensure loading and saving use the corresponding appCfg entries alongside the neighboring view and animation settings.
1350-1363: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCoordinate footer lags one frame behind.
app.mouseCoordText/cameraLookTextare updated byrenderSceneOverlay(), which runs insiderenderSceneWindow()— afterrenderMapViewerPanel()has already drawn them. Rendering the scene window first (or updating the strings before the panels) removes the one-frame lag.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mp2p_icp_viz/apps/mm-viewer/main.cpp` around lines 1350 - 1363, Update renderFrame so renderSceneWindow runs before renderMapViewerPanel, ensuring renderSceneOverlay refreshes app.mouseCoordText and cameraLookText before the coordinate footer is drawn; preserve the remaining panel rendering order unless required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agents.md`:
- Around line 145-146: Remove the machine-specific
`~/plans/mp2p_icp_imgui_port.md` reference from the migration note in
`agents.md`, or replace it with a repository-relative plan location only if that
plan is added to the repository. Keep the Dear ImGui migration context intact.
- Around line 156-157: Align the documented and declared MRPT dependency for
CImGuiSceneView with its actual provider: update the agents.md requirement and
mp2p_icp_viz/package.xml to require MRPT 2.15.11 or newer and include
mrpt_imgui. Keep the unconditional includes in mm-viewer and icp-log-viewer
valid, or instead guard those features consistently and document the guarded
behavior.
In `@mp2p_icp_viz/apps/icp-log-viewer/main.cpp`:
- Around line 632-642: Update onExport and the export-related logic in
renderManualPanel to catch exceptions from DelayedLoadLog::get() instead of
allowing them to escape the ImGui frame. Report load or export failures through
the existing UI error-display mechanism, while preserving the current successful
export behavior.
- Around line 838-861: Update the index-change handling in rebuild_3d_view() to
reseed app.gtPose from the newly selected record’s lr.icpResult.optimal_tf.mean
pose whenever selectorIdx changes. Populate all six components before
renderManualPanel() uses the sliders, preserving the selected record’s current
pose and preventing stale values across records.
- Around line 626-630: Update rebuild_3d_view and its per-frame call path to
latch the failed record index when DelayedLoadLog::get() throws, then skip
rebuilding while the current index matches that latched failure. Reset the
failure latch whenever navigation changes the record index, while preserving
rebuild attempts for new indices.
- Around line 943-950: Update the min-quality guard to check
filteredFiles.empty() instead of files.empty() before moving filteredFiles into
files. Preserve the existing exception message and threshold handling so the
specific error is raised when all logs are filtered out.
- Around line 160-163: Change the default value of pairingsCov2CovDecim to
approximately 2.7 so it is valid log10-space for the existing std::pow(10.0,
...) consumption and [0, 3] slider range. Keep the persisted and re-read
configuration default aligned with this corrected value.
In `@mp2p_icp_viz/apps/imgui_app_common/src/SimpleFileDialog.cpp`:
- Around line 127-134: Update the double-click handling in SimpleFileDialog’s
selectable-entry logic so it assigns result only when the dialog is in Open
mode; in Save mode, double-clicking should update typedName_ but require the
Save button’s existing validation and confirmation flow.
- Around line 89-102: Update the directory iteration in the file-listing routine
to use error-code-based status checks: call entry.exists(ec), skip the entry
when ec is set or it does not exist, then call entry.is_directory(ec) and skip
on any resulting error before classifying directories or filtered files.
Preserve the existing dirs/files population behavior for entries whose status
resolves successfully.
In `@mp2p_icp_viz/apps/mm-viewer/main.cpp`:
- Around line 941-955: Invalidate the cached visualization whenever a new map is
loaded, not only when render_params_t changes. Update loadMapFile() or
updateGuiAfterLoadingNewMap() to mark the cache stale, then make the rebuild
condition around prevRenderParams also consume that invalidation before calling
app.theMap.get_visualization().
- Around line 273-278: Update loadMapFile so it loads into a temporary
metric_map_t and commits it to app.theMap only after all loading steps succeed.
Keep the existing app.layerNames, layerVisible, and theMapFileName bookkeeping
unchanged on failure, while preserving the current success-path updates and GUI
refresh behavior.
- Around line 1285-1297: Update the keyframe UI around app.camTravellingLabels
and the "##travellingKeys" combo so it is not presented as an inert interactive
selector: either add AppState selection tracking and handle ImGui::Selectable
results with corresponding goto/delete actions, or replace the combo with a
plain scrolling list. Ensure the chosen approach accurately reflects the
available keyframe behavior instead of hard-wiring the preview to the last
entry.
- Around line 1463-1465: Validate appCfgFile immediately after
get_user_config_file() and handle an empty path before constructing CConfigFile.
When the path is empty, avoid persistence by skipping the associated config
read/write flow or use an in-memory configuration; ensure later appCfg.write
calls cannot target an empty filename while preserving normal behavior for valid
paths.
- Around line 419-441: Update transformAndFormatSelectedPoint so case 1 and case
2 convert map-frame points to ENU using
app.theMap.georeferencing->T_enu_to_map.mean.inverseComposePoint when
georeferencing exists and applyGeoRef is false, matching the conversion used by
case 0. Format the converted point in the ENU and lat-lon modes while preserving
existing behavior for already-ENU points.
- Around line 871-880: Update onSaveLayers() to export only layers marked in
app.layerVisible, matching each selected layer name from app.layerNames. Derive
a distinct output filename for every selected layer before calling
saveMetricMapRepresentationToFile(), so exports do not overwrite one another;
leave unselected layers untouched.
- Around line 1056-1066: Move the app.openDialog.render() and
app.exportDialog.render() calls outside the “Map viewer” ID stack so dialog
visibility is independent of that window’s collapse or hiding. Update each
SimpleFileDialog instance to use a distinct popup ID, preserving the existing
loadMapFile/updateGuiAfterLoadingNewMap and onSaveLayers handling for their
respective dialogs.
- Around line 754-755: Update the playback timing around t advancement to use
real elapsed time between rendered frames rather than 1/animFPS, while
preserving the requested animation speed. Also clamp animFPS immediately after
the InputFloat handling to a valid positive range (for example 1–240) so
negative values cannot prevent playback from reaching t1; update any remaining
animFPS uses consistently.
---
Nitpick comments:
In `@mp2p_icp_viz/apps/mm-viewer/main.cpp`:
- Around line 889-904: Update the visibility check in the layer-rendering loop
around app.layerVisible to use an explicit lookup instead of operator[]. Treat
missing layer entries as visible by default, while preserving the existing
hidden-layer continue behavior for entries explicitly set to false.
- Around line 259-271: Update rebuildCamTravellingLabels to iterate directly
over app.camTravelling with a single iterator-based pass, while maintaining a
separate incrementing index for the label number; remove the repeated begin()
and std::advance() calls so label generation is linear.
- Around line 92-159: Update mainShowGui() appCfg persistence to round-trip the
panel fields colorIntensityIdx, recolorizeByFieldIdx, mouseUnitsIdx, animFPS,
and travellingInterpIdx, including at minimum colorIntensityIdx. Keep their
existing AppState defaults and ensure loading and saving use the corresponding
appCfg entries alongside the neighboring view and animation settings.
- Around line 1350-1363: Update renderFrame so renderSceneWindow runs before
renderMapViewerPanel, ensuring renderSceneOverlay refreshes app.mouseCoordText
and cameraLookText before the coordinate footer is drawn; preserve the remaining
panel rendering order unless required.
🪄 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 Plus
Run ID: 6708ba0b-fff6-4aab-801e-4acdd35192a0
📒 Files selected for processing (17)
.gitignore.gitmodulesagents.mdmp2p_icp_viz/3rdparty/imguimp2p_icp_viz/3rdparty/imgui_static/CMakeLists.txtmp2p_icp_viz/CMakeLists.txtmp2p_icp_viz/apps/CMakeLists.txtmp2p_icp_viz/apps/icp-log-viewer/CMakeLists.txtmp2p_icp_viz/apps/icp-log-viewer/main.cppmp2p_icp_viz/apps/imgui_app_common/CMakeLists.txtmp2p_icp_viz/apps/imgui_app_common/include/imgui_app_common/ImGuiAppShell.hmp2p_icp_viz/apps/imgui_app_common/include/imgui_app_common/SimpleFileDialog.hmp2p_icp_viz/apps/imgui_app_common/src/ImGuiAppShell.cppmp2p_icp_viz/apps/imgui_app_common/src/SimpleFileDialog.cppmp2p_icp_viz/apps/mm-viewer/CMakeLists.txtmp2p_icp_viz/apps/mm-viewer/main.cppmp2p_icp_viz/package.xml
Comment on lines
+1463
to
+1465
| char appCfgFile[1024]; | ||
| ::get_user_config_file(appCfgFile, sizeof(appCfgFile), APP_NAME); | ||
| mrpt::config::CConfigFile appCfg(appCfgFile); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unchecked config path from get_user_config_file().
get_user_config_file() writes an empty string when HOME/XDG_CONFIG_HOME are unset or the buffer is too small (see mp2p_icp_viz/apps/libcfgpath/cfgpath.h). CConfigFile("") then operates on an empty filename, and the later appCfg.write(...) calls at Lines 1499-1522 will attempt to save it. Guard for the empty path and skip persistence (or fall back to an in-memory config).
🛡️ Proposed guard
char appCfgFile[1024];
::get_user_config_file(appCfgFile, sizeof(appCfgFile), APP_NAME);
+ if (appCfgFile[0] == '\0')
+ {
+ std::cerr << "Warning: could not determine a user config file path; "
+ "UI settings will not be persisted.\n";
+ }
mrpt::config::CConfigFile appCfg(appCfgFile);(and wrap the read/write blocks accordingly).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mp2p_icp_viz/apps/mm-viewer/main.cpp` around lines 1463 - 1465, Validate
appCfgFile immediately after get_user_config_file() and handle an empty path
before constructing CConfigFile. When the path is empty, avoid persistence by
skipping the associated config read/write flow or use an in-memory
configuration; ensure later appCfg.write calls cannot target an empty filename
while preserving normal behavior for valid paths.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mp2p_icp_viz/apps/imgui_app_common/src/SimpleFileDialog.cpp (2)
194-203: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not silently overwrite Save targets.
Save mode returns success for an existing path without confirmation. The downstream
mm-viewer::onSaveLayers()writes derived per-layer files, so existing exports can be replaced without an explicit user decision. Add overwrite confirmation or reject existing output targets, including the derived files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mp2p_icp_viz/apps/imgui_app_common/src/SimpleFileDialog.cpp` around lines 194 - 203, Update the path-selection logic around candidate and the mode_ checks so Save mode does not return success when the target already exists. Require explicit overwrite confirmation before accepting existing targets, and validate the derived per-layer output files used by mm-viewer::onSaveLayers() rather than checking only the base path.
194-198: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire a regular file and avoid throwing filesystem checks.
exists(candidate)will also succeed for directories and the throwlessexistsoverload is not being used, so permission/status problems can still bubble out of the GUI. Use theerror_codeoverload andis_regular_filefor Open mode, and report the filesystem error explicitly.Proposed fix
+ std::error_code candidateEc; - if (mode_ == Mode::Open && !std::filesystem::exists(candidate)) + if (mode_ == Mode::Open && + !std::filesystem::is_regular_file(candidate, candidateEc)) { - errorMsg_ = "File does not exist."; + errorMsg_ = candidateEc + ? "Unable to inspect file." + : "File does not exist or is not a regular file."; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mp2p_icp_viz/apps/imgui_app_common/src/SimpleFileDialog.cpp` around lines 194 - 198, Update the Open-mode validation in the typedName_ candidate handling to use non-throwing filesystem status checks with a std::error_code, require candidate to be a regular file via is_regular_file, and set errorMsg_ to explicitly report any filesystem error returned by the checks. Preserve the existing success path for valid regular files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@mp2p_icp_viz/apps/imgui_app_common/src/SimpleFileDialog.cpp`:
- Around line 194-203: Update the path-selection logic around candidate and the
mode_ checks so Save mode does not return success when the target already
exists. Require explicit overwrite confirmation before accepting existing
targets, and validate the derived per-layer output files used by
mm-viewer::onSaveLayers() rather than checking only the base path.
- Around line 194-198: Update the Open-mode validation in the typedName_
candidate handling to use non-throwing filesystem status checks with a
std::error_code, require candidate to be a regular file via is_regular_file, and
set errorMsg_ to explicitly report any filesystem error returned by the checks.
Preserve the existing success path for valid regular files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e2f8459c-c61d-43cb-8e7a-64fb013308de
📒 Files selected for processing (6)
.github/workflows/ros-build.ymlagents.mdmp2p_icp_viz/apps/icp-log-viewer/main.cppmp2p_icp_viz/apps/imgui_app_common/include/imgui_app_common/SimpleFileDialog.hmp2p_icp_viz/apps/imgui_app_common/src/SimpleFileDialog.cppmp2p_icp_viz/apps/mm-viewer/main.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- mp2p_icp_viz/apps/imgui_app_common/include/imgui_app_common/SimpleFileDialog.h
- agents.md
- mp2p_icp_viz/apps/icp-log-viewer/main.cpp
- mp2p_icp_viz/apps/mm-viewer/main.cpp
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #79 +/- ##
========================================
Coverage 72.07% 72.07%
========================================
Files 155 155
Lines 6790 6790
Branches 861 861
========================================
Hits 4894 4894
Misses 1896 1896 🚀 New features to boost your workflow:
|
mrpt::imgui::CImGuiSceneView.h (now used directly by mp2p_icp_viz) pulls in mrpt/opengl/opengl_api.h, which unconditionally includes <GL/glut.h> on Linux. There is no rosdep key for freeglut3-dev, so rosdep install silently skips it and the build fails at the first translation unit that touches the header. Install it directly via apt, matching the existing pattern for other non-rosdep packages in this workflow. missing glut dep for mrpt2
SimpleFileDialog: - Give each instance a unique popup ID (derived from `this`); mm-viewer's openDialog/exportDialog previously shared the same hardcoded ID. - Move dialog rendering to top level in both apps, outside any other window's Begin/End -- nesting it meant collapsing that window made BeginPopupModal() fail to start, which was treated as a dismiss. - Use the error-code overload of is_directory() during listing instead of the throwing one. - Save mode no longer confirms on double-click (only Open does); Save always goes through the Save button and its checks. icp-log-viewer: - Fix pairingsCov2CovDecim default (500.0f instead of its log10, ~2.7): consumed as pow(10, x) with a slider range of [0,3], so until the slider was touched this overflowed to inf and then to UB in the static_cast to size_t. - Fix the min-quality filter checking the wrong (unfiltered) list, so the "no logs passed" error never actually fired. - Guard against repeatedly retrying (and re-logging) a record that failed to load, and stop it from being able to throw out of the ImGui frame from onExport()/renderManualPanel(). - Seed the manual pose-nudge sliders from each record's own pose instead of leaving stale values from whatever record was open before. mm-viewer: - onSaveLayers() now honors layer visibility (matching its "marked layers" label) and writes one file per layer instead of clobbering the same path for every layer. - Fix enu/lat-lon mouse-coordinate readout when a georeferenced map is loaded but "Apply georeferenced pose" is unchecked. - Force point-cloud regeneration when a new map is loaded, instead of only on render_params_t changes (avoids showing stale geometry if a different file happens to produce an equal render_params_t). - Clear layer bookkeeping alongside the map on a failed load. - Render the keyframe list as plain read-only text instead of a combo that looked interactive but ignored selection. - Clamp the animation FPS input; warn (instead of silently no-op-ing) if no user config directory could be resolved. agents.md: correct the MRPT minimum for CImGuiSceneView (2.15.11, not 2.15.19) and drop the machine-local plan file path.
…operly Save mode returned success for an existing path with no confirmation -- unlike a native OS save dialog (which the old nanogui app got automatically via nanogui::file_dialog()), this in-app dialog had no overwrite protection of its own. Add a small nested confirmation popup, only reached when the target already exists. Also switch the Open-mode existence check to the non-throwing is_regular_file() overload (instead of exists(), which also passes for directories and has a throwing default overload).
…hread Loading a large .mm file (real-world example: a 938MB / 35M-point map) blocked the main/GL thread for the whole duration of both the file I/O and the subsequent theMap.get_visualization() call. Since that thread also owns the GLFW event loop, it stopped calling glfwPollEvents() for long enough that the window manager concluded the process was hung and offered to kill it, even though the app would have kept responding once given the chance. Adds a small reusable AsyncTask<T> helper (imgui_app_common) that runs a computation via std::async and is polled once per frame instead of blocked on. loadMapFile() is split into a self-contained worker (safe to run off-thread, touches no AppState) plus a cheap main-thread commit step; get_visualization() is similarly dispatched on a background thread, working off a shallow copy of the map (cheap: shares the underlying layer pointers) and tagged with a generation counter so a stale result (superseded by a newer map load while it was still building) is discarded instead of overwriting the current view. The CLI-arg initial load stays synchronous: no GUI window exists yet at that point, so there is nothing for the window manager to watch. Verified against a real 938MB/35M-point map (loads, builds its full visualization, and stays interactive throughout) and against the small demo map (no regression).
jlblancoc
force-pushed
the
feat/imgui-viewers-port
branch
from
July 28, 2026 05:56
2e8da2a to
d597e00
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mp2p_icp_viz/apps/imgui_app_common/include/imgui_app_common/AsyncTask.h`:
- Around line 25-28: Update AsyncTask::start to reject calls while a task is
already running, returning a success/failure result without replacing the active
future; only launch and mark running for accepted starts. Adjust the
startMapLoad callers for mapLoadTask and vizBuildTask to handle the rejected
result and preserve the existing load flow without blocking or overwriting an
in-progress task.
🪄 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 Plus
Run ID: d1fe20e7-2df3-4340-833c-593efbec800e
📒 Files selected for processing (9)
.github/workflows/ros-build.ymlagents.mdmp2p_icp_viz/apps/icp-log-viewer/main.cppmp2p_icp_viz/apps/imgui_app_common/CMakeLists.txtmp2p_icp_viz/apps/imgui_app_common/include/imgui_app_common/AsyncTask.hmp2p_icp_viz/apps/imgui_app_common/include/imgui_app_common/SimpleFileDialog.hmp2p_icp_viz/apps/imgui_app_common/src/SimpleFileDialog.cppmp2p_icp_viz/apps/mm-viewer/main.cppmp2p_icp_viz/package.xml
🚧 Files skipped from review as they are similar to previous changes (7)
- mp2p_icp_viz/package.xml
- mp2p_icp_viz/apps/imgui_app_common/CMakeLists.txt
- mp2p_icp_viz/apps/imgui_app_common/include/imgui_app_common/SimpleFileDialog.h
- agents.md
- mp2p_icp_viz/apps/imgui_app_common/src/SimpleFileDialog.cpp
- mp2p_icp_viz/apps/icp-log-viewer/main.cpp
- mp2p_icp_viz/apps/mm-viewer/main.cpp
Comment on lines
+25
to
+28
| void start(std::function<T()> fn) | ||
| { | ||
| future_ = std::async(std::launch::async, std::move(fn)); | ||
| running_ = true; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify every AsyncTask start site is protected against overlapping work.
rg -n -C 5 '\.(mapLoadTask|vizBuildTask)\.start\(' mp2p_icp_viz/apps
rg -n -C 5 'startMapLoad|isRunning\(\)' mp2p_icp_viz/appsRepository: MOLAorg/mp2p_icp
Length of output: 5740
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== AsyncTask.h =="
cat -n mp2p_icp_viz/apps/imgui_app_common/include/imgui_app_common/AsyncTask.h
echo
echo "== mapLoadTask declarations/usages =="
rg -n -C 4 'mapLoadTask|vizBuildTask|isLoadingMap|isBuildingViz|startMapLoad|pollMapLoad|pollBuildTask' mp2p_icp_viz/apps/mm-viewer/main.cpp
echo
echo "== behavioral probe: std::future assignment under std::launch::async task running =="
cat > /tmp/async_reassign_probe.cpp <<'CPP'
`#include` <future>
`#include` <mutex>
`#include` <condition_variable>
`#include` <chrono>
`#include` <iostream>
void no_move_test()
{
bool running = false;
std::mutex m;
std::condition_variable wake_for_release, done;
{
std::future<void> future = std::async(std::launch::async, [&](){
{
std::lock_guard<std::mutex> lock(m);
running = true;
wake_for_release.wait(lock);
}
done.notify_all();
});
{
std::unique_lock<std::mutex> lock(m);
done.wait(lock, [&]{ return running; });
}
std::cerr << "waiting_before_assignment:";
auto start = std::chrono::steady_clock::now();
std::future<void> replacement;
replacement = std::async(std::launch::async, [](){ std::cerr << "not_reached\n"; });
auto end = std::chrono::steady_clock::now();
std::cerr << " duration_ms="
<< std::chrono::duration_cast<std::chrono::milliseconds>(end - start).count()
<< "\n";
replacement = std::move(future);
}
}
int main()
{
no_move_test();
}
CPP
if command -v c++ >/dev/null 2>&1; then
c++ -std=c++17 -pthread /tmp/async_reassign_probe.cpp -o /tmp/async_reassign_probe
echo "running /tmp/async_reassign_probe"
timeout 10 /tmp/async_reassign_probe || true
else
echo "c++ unavailable"
fiRepository: MOLAorg/mp2p_icp
Length of output: 8621
🌐 Web query:
C++ std::future destructor wait for ready state std::launch::async documentation
💡 Result:
In C++, the destructor of a std::future object typically does not block, with one specific exception: when the future is associated with a shared state created by a call to std::async using the std::launch::async policy [1][2][3]. If a std::future is obtained from std::async and satisfies the following three conditions, its destructor will block until the asynchronous task completes [1][3]: 1. The shared state was created by a call to std::async [1][3]. 2. The shared state is not yet ready (the task has not completed) [1][3]. 3. The std::future object is the last reference to that shared state (i.e., it has not been moved from) [1][3]. This behavior essentially makes the asynchronous operation synchronous for temporary std::future objects [2][4]. For example, the following code will execute sequentially because the destructor of the first future blocks until the task completes before the second line is executed: std::async(std::launch::async, []{ /* task 1 / }); // Destructor blocks here until task 1 finishes std::async(std::launch::async, []{ / task 2 */ }); // Task 2 does not start until task 1 is done This blocking mechanism exists to ensure that the associated thread, which may depend on local variables in the scope where std::async was called, is properly joined and does not access invalid memory after the scope ends [5]. Destructors of std::futures obtained through other means, such as std::promise or std::packaged_task, do not block [2][4].
Citations:
- 1: https://en.cppreference.com/cpp/thread/future/~future
- 2: https://en.cppreference.com/cpp/thread/async
- 3: http://en.cppreference.com/w/cpp/thread/future/~future
- 4: https://cppreference.com/cpp/thread/async
- 5: https://stackoverflow.com/questions/23455104/why-is-the-destructor-of-a-future-returned-from-stdasync-blocking
Guard AsyncTask against replacing an active future.
start() assigns to future_ before clearing running_; when that task is still pending and its shared state has no other references, std::future destructor assignment can block on the async thread completion. If startMapLoad() is called twice before pollMapLoad() completes, the UI load path can stall for the previous map load. Reject overlapping start() calls explicitly and update mapLoadTask.start() / vizBuildTask.start() callers to handle the result.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mp2p_icp_viz/apps/imgui_app_common/include/imgui_app_common/AsyncTask.h`
around lines 25 - 28, Update AsyncTask::start to reject calls while a task is
already running, returning a success/failure result without replacing the active
future; only launch and mark running for accepted starts. Adjust the
startMapLoad callers for mapLoadTask and vizBuildTask to handle the rejected
result and preserve the existing load flow without blocking or overwriting an
in-progress task.
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
Migrates both GUI apps in
mp2p_icp_viz(mm-viewer,icp-log-viewer) frommrpt::gui::CDisplayWindowGUI+ nanogui to Dear ImGui (docking branch), on top ofmrpt::imgui::CImGuiSceneView(ships withmrpt::gui>= 2.15.19).mp2p_icp_viz/3rdparty/imgui, docking branch), built as a private static lib that is never installed/exported.mp2p_icp_viz/apps/imgui_app_common/:ImGuiAppShell(GLFW/GL/docking boilerplate + asetupDefaultLayouthook so each app defines its own default panel layout) andSimpleFileDialog(self-contained ImGui file browser, no native/system dialog dependency).~/.config/<app>/).CImGuiSceneViewonly renders the"main"viewport) — documented inagents.mdsince MRPT 3.x is expected to support this and the code should start working again unmodified.See
~/plans/mp2p_icp_imgui_port.md(not part of this repo) for the full phased plan/status.Test plan
colcon build --packages-select mp2p_icp_vizbuilds cleanfind install -iname '*imgui*'returns nothing (imgui never installed/exported)mm-viewermanually run againstmp2p_icp_core/demos/global_001.mm: load, layer toggles, recolorization, camera, default docking layout all verified visuallyicp-log-viewermanually run against a real.icploggenerated viaicp-run -d: record load, stats, dynamic variables, pairings, default docking layout all verified visuallyclang-format-14 --dry-run --Werrorclean viamp2p_icp_core/scripts/formatter.sh --checkSummary by CodeRabbit