Introduce Dear ImGui alternative viz module#112
Open
Conversation
📝 WalkthroughWalkthroughAdds a new Dear ImGui–based visualization backend (mola::MolaVizImGui) with CMake build, ImGui submodule/static target, handlers/widgets implementations, demo config updates to select the new backend, package metadata, and minor README/comment tweaks. Changes
Sequence DiagramsequenceDiagram
participant User
participant MainThread as Main Thread
participant TaskQueue as Pending Tasks
participant GUIThread as GUI Thread (GLFW/ImGui)
participant Renderer as OpenGL Renderer
User->>MainThread: enqueue_custom_gui_code(task)
MainThread->>TaskQueue: push task + promise
MainThread-->>User: return future
GUIThread->>TaskQueue: poll & pop task
GUIThread->>GUIThread: execute task (ImGui calls / scene updates)
GUIThread->>Renderer: update 3D scene / decaying clouds
GUIThread->>GUIThread: render ImGui windows / dockspace / console
GUIThread->>Renderer: SwapBuffers (GLFW)
GUIThread->>TaskQueue: fulfill promise (future ready)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 6
🧹 Nitpick comments (6)
mola_viz/README.md (1)
2-2: Good clarification to specify the GUI backend.The addition of "based on nanogui" appropriately distinguishes this module from the new Dear ImGui-based alternative being introduced in this PR. This improves clarity for users choosing between the two backends.
Optional enhancement: Cross-reference the ImGui alternative
Consider adding a brief note mentioning that an ImGui-based alternative (
mola_viz_imgui) is also available, helping users discover both options. For example:# mola_viz GUI for MOLA based on nanogui + +> **Note:** An alternative Dear ImGui-based backend is available in the `mola_viz_imgui` module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mola_viz/README.md` at line 2, Update the README entry for mola_viz to briefly cross-reference the new ImGui alternative by adding a single line note like "For an ImGui-based alternative see mola_viz_imgui" so users can discover both backends; modify the top-level description in README.md (the line containing "GUI for MOLA based on nanogui") to include this short pointer and, if desired, a parenthetical or link to the mola_viz_imgui module name.mola_viz_imgui/3rdparty/imgui (1)
1-1: Consider pinning to a tagged release instead of a commit hash.Using a specific commit hash makes it difficult to track which version of Dear ImGui is being used. Consider pinning to a tagged release (e.g.,
v1.90.0-docking) for better traceability, easier updates, and clearer dependency management.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mola_viz_imgui/3rdparty/imgui` at line 1, Update the Dear ImGui dependency in mola_viz_imgui/3rdparty/imgui to reference a tagged release (e.g., v1.90.0-docking) instead of a raw commit hash: change the submodule/reference to point to the tag (update .gitmodules or the place where the commit is pinned), fetch the tag from the upstream repo and update the submodule to that tag (git fetch --tags; git checkout refs/tags/v1.90.0-docking; git submodule update --init), and commit the updated submodule reference so the project now tracks the clear release tag rather than a specific commit.mola_viz_imgui/src/MolaVizImGui_handlers.cpp (1)
50-66: Thread-safety concern with static maps inshow_common_sensor_info.The static
std::mapinstances (lastTimestamp,estimatedHz) are modified without synchronization. Ifshow_common_sensor_infois ever called from multiple threads (e.g., multiple handler invocations concurrently), this could lead to data races.Given the context that all ImGui calls should happen on the GUI thread (per the header documentation), this is likely safe in practice. However, consider adding a comment clarifying this assumption, or using
thread_localto make it explicit.💡 Suggested clarification
void show_common_sensor_info(const mrpt::obs::CObservation& obs, const std::string& subWindowTitle) { // Rate estimation — one low-pass filter per (subWindowTitle, sensor class): + // Note: safe without locking because all handlers run on the GUI thread. using key_t = std::pair<std::string, std::string>; static std::map<key_t, double> lastTimestamp; static std::map<key_t, double> estimatedHz;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mola_viz_imgui/src/MolaVizImGui_handlers.cpp` around lines 50 - 66, The static maps lastTimestamp and estimatedHz used in show_common_sensor_info (key_t, lastTimestamp, estimatedHz) are modified without synchronization and could race if called concurrently; either mark them thread_local to confine them to each thread or (preferable given ImGui constraints) add a clear comment above their declarations stating that show_common_sensor_info must only be called on the GUI thread and that these statics rely on that invariant, so future maintainers understand the thread-safety assumption. Ensure the chosen fix references the existing symbols (lastTimestamp, estimatedHz, key_t, show_common_sensor_info) so maintainers can locate and apply it.mola_viz_imgui/src/MolaVizImGui.cpp (1)
824-833: Considerstd::dequefor console messages.The
console_messagescontainer usesstd::vectorwitherase(begin())to remove old messages. This is O(n) per removal. Whilemax_console_lines_defaults to 12 (making this negligible),std::dequewould be more efficient for this FIFO pattern and consistent withdecaying_cloudswhich already usesstd::deque.💡 Suggested change in header
In
PerWindowData(MolaVizImGui.h):- std::vector<std::string> console_messages; + std::deque<std::string> console_messages;Then in this code:
- wd.console_messages.erase(wd.console_messages.begin()); + wd.console_messages.pop_front();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mola_viz_imgui/src/MolaVizImGui.cpp` around lines 824 - 833, Replace PerWindowData::console_messages from std::vector<std::string> to std::deque<std::string> and update the usage in MolaVizImGui.cpp: push new lines with wd.console_messages.push_back(line) (unchanged) and drop old entries using wd.console_messages.pop_front() or while (wd.console_messages.size() > max_console_lines_) wd.console_messages.pop_front();; ensure any other code referencing console_messages still compiles (similar to existing decaying_clouds usage) and adjust includes if needed to include <deque>.mola_viz_imgui/src/MolaVizImGui_widgets.cpp (2)
249-255: Fragile: ComboBox ID derived from stack address when label is empty.When
widget.labelis empty, the ID is derived from&widget. Sincewidgetis a reference to an element stored inSubWindowState::desc, this works as long as the description isn't reallocated. However, this is fragile and could break if theWindowDescriptionis ever moved or recreated.Consider using a stable identifier such as a sequential index or the widget's position in the tab.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mola_viz_imgui/src/MolaVizImGui_widgets.cpp` around lines 249 - 255, The current Combo ID uses the stack address of widget (reinterpret_cast<uintptr_t>(&widget)) which is fragile; instead generate a stable ID for ImGui::Combo when widget.label is empty by using a persistent identifier such as the widget's index or position in the parent container (e.g., the loop index over SubWindowState::desc or a dedicated widget_id stored on the WindowDescription), and build the id string from that stable value (replace creation of id string where id is assigned and passed to ImGui::Combo). Ensure the new id scheme uniquely identifies the widget across reorders/reallocations and keep the existing branch that uses widget.label when non-empty.
127-139: Static state maps keyed by label can collide across windows.As noted in the comment, the static
std::map<std::string, bool> stateswill collide if the same label is used in different windows or tabs. This applies to all widget types using this pattern (CheckBox, TextBox, SliderFloat, SliderInt, ComboBox).The comment suggests storing the state in
SubWindowStateinstead. For a more robust solution, consider keying by a combination of widget identity (e.g., usingLiveString::id()pattern or the containingWindowDescription's title).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mola_viz_imgui/src/MolaVizImGui_widgets.cpp` around lines 127 - 139, The static map std::map<std::string,bool> states used in the Checkbox block (and the equivalent maps for TextBox, SliderFloat, SliderInt, ComboBox) can collide across windows; replace the static per-process map with per-window storage (store the bool in SubWindowState) or key state by a unique widget identity (e.g., combine the containing WindowDescription title + widget.label or use the widget's LiveString::id()) and then read/write that per-window/subwindow state when calling ImGui::Checkbox(widget.label.c_str(), &value) and invoking widget.on_change(value); update the code paths referencing widget.label, widget.initial_value and widget.on_change to use the new per-window/subwindow state container instead of the static map.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mola_viz_imgui/3rdparty/imgui_static/CMakeLists.txt`:
- Around line 50-54: The CMakeLists sets the compile definition
IMGUI_IMPL_OPENGL_LOADER_GLEW for the target imgui_static but never finds or
links GLEW; either remove that definition so imgui uses its default loader or
switch it to a loader you actually provide (e.g., IMGUI_IMPL_OPENGL_LOADER_GLAD)
and then add the corresponding find_package() and
target_link_libraries(imgui_static ...) entries (or add find_package(GLEW) and
link GLEW) to ensure the declared loader macro matches an available dependency.
In `@mola_viz_imgui/CMakeLists.txt`:
- Around line 56-58: The imgui dependency is currently added under
PRIVATE_LINK_LIBRARIES which prevents transitive propagation to consumers that
include the public header MolaVizImGui.h (which includes <imgui.h>,
<imgui_impl_glfw.h>, <imgui_impl_opengl3.h>, <GLFW/glfw3.h>); change the linkage
for the imgui::imgui target from PRIVATE_LINK_LIBRARIES to PUBLIC_LINK_LIBRARIES
in the CMakeLists so the imgui target's include directories and link interface
are exported to downstream targets and compilation of consumers succeeds.
In `@mola_viz_imgui/src/MolaVizImGui_handlers.cpp`:
- Around line 98-105: The ImGui window ID and sensor key are hardcoded causing
collisions; update the handler that renders observations (uses ImGui::Begin and
show_common_sensor_info) to derive a unique ID/key from the observation's
sensorLabel (cast via std::dynamic_pointer_cast<mrpt::obs::CObservation>) or the
provided subWindowTitle/parentWin instead of using "##img_stub" and "image";
build a unique string (e.g., "##img_"+obs->sensorLabel) and pass
obs->sensorLabel (or the composed unique key) into show_common_sensor_info so
multiple CObservationImage / CObservation3DRangeScan instances maintain separate
ImGui windows and rate state.
In `@mola_viz_imgui/src/MolaVizImGui.cpp`:
- Around line 92-97: The MRPT initializer do_register_MolaVizImGui registers the
module but never registers the default sensor-observation GUI handlers; add a
forward declaration for void mola_viz_imgui_register_default_handlers() near the
top of this file (or in the header) and call
mola_viz_imgui_register_default_handlers() inside
MRPT_INITIALIZER(do_register_MolaVizImGui) after
MOLA_REGISTER_MODULE(MolaVizImGui) so GPS/IMU/image/point-cloud handlers are
activated.
- Around line 845-866: subwindow_update_visualization captures the raw pointer
extra_parameters into a packaged_task which may run later, creating a dangling
pointer risk; fix by making a safe copy of the yaml before creating the lambda
(e.g., if extra_parameters != nullptr create a std::shared_ptr<const
mrpt::containers::yaml> or a value copy and capture that instead) and then
capture the new shared_ptr/value in the packaged_task lambda used when pushing
into guiThreadPendingTasks_, ensuring the lambda no longer holds a raw pointer
to external memory.
In `@mola_viz/src/MolaViz.cpp`:
- Around line 1930-1931: The log stream in the exception handler for
execute_custom_code_on_background_scene() is misformatted for clang-format;
reflow the std::cerr/std::ostringstream (or the insertion chain shown) into a
single properly indented statement so the message and e.what() are on the same
line and the closing parentheses/semicolon align with surrounding code style
(e.g., combine the string literal and e.what() insertion into one continuous
insertion expression). Locate the exception logging code that contains
"Exception in execute_custom_code_on_background_scene():\n" and adjust
spacing/line breaks to match the project's clang-format rules.
---
Nitpick comments:
In `@mola_viz_imgui/3rdparty/imgui`:
- Line 1: Update the Dear ImGui dependency in mola_viz_imgui/3rdparty/imgui to
reference a tagged release (e.g., v1.90.0-docking) instead of a raw commit hash:
change the submodule/reference to point to the tag (update .gitmodules or the
place where the commit is pinned), fetch the tag from the upstream repo and
update the submodule to that tag (git fetch --tags; git checkout
refs/tags/v1.90.0-docking; git submodule update --init), and commit the updated
submodule reference so the project now tracks the clear release tag rather than
a specific commit.
In `@mola_viz_imgui/src/MolaVizImGui_handlers.cpp`:
- Around line 50-66: The static maps lastTimestamp and estimatedHz used in
show_common_sensor_info (key_t, lastTimestamp, estimatedHz) are modified without
synchronization and could race if called concurrently; either mark them
thread_local to confine them to each thread or (preferable given ImGui
constraints) add a clear comment above their declarations stating that
show_common_sensor_info must only be called on the GUI thread and that these
statics rely on that invariant, so future maintainers understand the
thread-safety assumption. Ensure the chosen fix references the existing symbols
(lastTimestamp, estimatedHz, key_t, show_common_sensor_info) so maintainers can
locate and apply it.
In `@mola_viz_imgui/src/MolaVizImGui_widgets.cpp`:
- Around line 249-255: The current Combo ID uses the stack address of widget
(reinterpret_cast<uintptr_t>(&widget)) which is fragile; instead generate a
stable ID for ImGui::Combo when widget.label is empty by using a persistent
identifier such as the widget's index or position in the parent container (e.g.,
the loop index over SubWindowState::desc or a dedicated widget_id stored on the
WindowDescription), and build the id string from that stable value (replace
creation of id string where id is assigned and passed to ImGui::Combo). Ensure
the new id scheme uniquely identifies the widget across reorders/reallocations
and keep the existing branch that uses widget.label when non-empty.
- Around line 127-139: The static map std::map<std::string,bool> states used in
the Checkbox block (and the equivalent maps for TextBox, SliderFloat, SliderInt,
ComboBox) can collide across windows; replace the static per-process map with
per-window storage (store the bool in SubWindowState) or key state by a unique
widget identity (e.g., combine the containing WindowDescription title +
widget.label or use the widget's LiveString::id()) and then read/write that
per-window/subwindow state when calling ImGui::Checkbox(widget.label.c_str(),
&value) and invoking widget.on_change(value); update the code paths referencing
widget.label, widget.initial_value and widget.on_change to use the new
per-window/subwindow state container instead of the static map.
In `@mola_viz_imgui/src/MolaVizImGui.cpp`:
- Around line 824-833: Replace PerWindowData::console_messages from
std::vector<std::string> to std::deque<std::string> and update the usage in
MolaVizImGui.cpp: push new lines with wd.console_messages.push_back(line)
(unchanged) and drop old entries using wd.console_messages.pop_front() or while
(wd.console_messages.size() > max_console_lines_)
wd.console_messages.pop_front();; ensure any other code referencing
console_messages still compiles (similar to existing decaying_clouds usage) and
adjust includes if needed to include <deque>.
In `@mola_viz/README.md`:
- Line 2: Update the README entry for mola_viz to briefly cross-reference the
new ImGui alternative by adding a single line note like "For an ImGui-based
alternative see mola_viz_imgui" so users can discover both backends; modify the
top-level description in README.md (the line containing "GUI for MOLA based on
nanogui") to include this short pointer and, if desired, a parenthetical or link
to the mola_viz_imgui module name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ac4f1af-497d-4b60-9933-4172883a2118
📒 Files selected for processing (15)
.gitmodulesmola_demos/mola-cli-launchs/kitti360_just_replay.yamlmola_demos/mola-cli-launchs/kitti_just_replay.yamlmola_viz/README.mdmola_viz/src/MolaViz.cppmola_viz_imgui/3rdparty/.clang-formatmola_viz_imgui/3rdparty/imguimola_viz_imgui/3rdparty/imgui_static/CMakeLists.txtmola_viz_imgui/CMakeLists.txtmola_viz_imgui/README.mdmola_viz_imgui/include/mola_viz_imgui/MolaVizImGui.hmola_viz_imgui/package.xmlmola_viz_imgui/src/MolaVizImGui.cppmola_viz_imgui/src/MolaVizImGui_handlers.cppmola_viz_imgui/src/MolaVizImGui_widgets.cpp
Comment on lines
+50
to
+54
| target_compile_definitions(imgui_static | ||
| PUBLIC | ||
| IMGUI_ENABLE_DOCKING # activates docking API | ||
| IMGUI_IMPL_OPENGL_LOADER_GLEW # or GLAD — adjust to whatever mrpt uses | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, verify the loader macro is set at lines 50-54
echo "=== Checking CMakeLists.txt at specified lines ==="
nl -ba mola_viz_imgui/3rdparty/imgui_static/CMakeLists.txt | sed -n '48,60p'
echo ""
echo "=== Searching for OpenGL loader macros and GLEW declarations ==="
rg -n "IMGUI_IMPL_OPENGL_LOADER_GLEW|find_package\(GLEW|GLEW::|libglew|glew" \
-g 'CMakeLists.txt' -g 'package.xml' 2>/dev/null || echo "No matches found"
echo ""
echo "=== Broader search for GLEW in CMakeLists files ==="
rg -i "glew" -g 'CMakeLists.txt' -n || echo "No GLEW mentions found"Repository: MOLAorg/mola
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if the file exists and read specified lines
echo "=== Checking CMakeLists.txt at specified lines ==="
if [ -f "mola_viz_imgui/3rdparty/imgui_static/CMakeLists.txt" ]; then
sed -n '48,60p' mola_viz_imgui/3rdparty/imgui_static/CMakeLists.txt | cat -n
else
echo "File not found: mola_viz_imgui/3rdparty/imgui_static/CMakeLists.txt"
echo "Checking available structure:"
fd -t f "CMakeLists.txt" | head -20
fi
echo ""
echo "=== Searching for OpenGL loader macros and GLEW declarations ==="
rg -n "IMGUI_IMPL_OPENGL_LOADER_GLEW|find_package.*GLEW|GLEW::|libglew" \
-g 'CMakeLists.txt' -g 'package.xml' 2>/dev/null || echo "No direct GLEW matches found"
echo ""
echo "=== Broader case-insensitive search for GLEW in CMakeLists files ==="
rg -i "glew" -g 'CMakeLists.txt' -n || echo "No GLEW mentions found"
echo ""
echo "=== Search for target_link_libraries in imgui CMakeLists ==="
rg -A 5 "target_link_libraries.*imgui_static" mola_viz_imgui/3rdparty/imgui_static/CMakeLists.txt || echo "No target_link_libraries found"Repository: MOLAorg/mola
Length of output: 1023
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Parent directory structure ==="
ls -la mola_viz_imgui/3rdparty/imgui_static/ | head -20
echo ""
echo "=== Full CMakeLists.txt for imgui_static ==="
cat mola_viz_imgui/3rdparty/imgui_static/CMakeLists.txt
echo ""
echo "=== Check parent CMakeLists for GLEW ==="
if [ -f "mola_viz_imgui/CMakeLists.txt" ]; then
echo "Found mola_viz_imgui/CMakeLists.txt:"
rg -i "glew|IMGUI_IMPL_OPENGL" mola_viz_imgui/CMakeLists.txt -n || echo "No GLEW or IMGUI_IMPL in parent"
fi
echo ""
echo "=== Search for imgui backend implementation files ==="
find mola_viz_imgui -name "*imgui_impl*" -type f 2>/dev/null | head -10Repository: MOLAorg/mola
Length of output: 2607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check for GLEW usage in entire project ==="
rg -i "glew" --type cmake -n | head -20 || echo "No GLEW in CMake files"
echo ""
echo "=== Check for other OpenGL loaders (GLAD, GL3W) ==="
rg -i "IMGUI_IMPL_OPENGL_LOADER_GLAD|IMGUI_IMPL_OPENGL_LOADER_GL3W|GLAD|GL3W" --type cmake -n | head -20
echo ""
echo "=== Check for MRPT or mrpt references ==="
rg -i "mrpt" mola_viz_imgui --type cmake -n
echo ""
echo "=== Check top-level CMakeLists or package.xml for dependencies ==="
if [ -f "CMakeLists.txt" ]; then
echo "Top-level CMakeLists.txt snippet:"
head -50 CMakeLists.txt
fi
echo ""
echo "=== Check for package.xml ==="
if [ -f "package.xml" ]; then
cat package.xml | head -40
fiRepository: MOLAorg/mola
Length of output: 1551
OpenGL loader macro is inconsistent with declared dependencies.
IMGUI_IMPL_OPENGL_LOADER_GLEW is enabled, but GLEW is neither found via find_package() nor linked in target_link_libraries(). The imgui OpenGL3 backend requires this dependency to compile. This will fail unless GLEW is provided by a parent project or the macro is changed to match an available loader.
Fix: Use GLAD or remove to use default
If GLEW is unavailable, either switch to GLAD (if available) or remove the macro line to use the imgui default loader selection:
target_compile_definitions(imgui_static
PUBLIC
IMGUI_ENABLE_DOCKING # activates docking API
- IMGUI_IMPL_OPENGL_LOADER_GLEW # or GLAD — adjust to whatever mrpt uses
)Alternatively, add GLEW dependency if it's required by MRPT or another dependency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mola_viz_imgui/3rdparty/imgui_static/CMakeLists.txt` around lines 50 - 54,
The CMakeLists sets the compile definition IMGUI_IMPL_OPENGL_LOADER_GLEW for the
target imgui_static but never finds or links GLEW; either remove that definition
so imgui uses its default loader or switch it to a loader you actually provide
(e.g., IMGUI_IMPL_OPENGL_LOADER_GLAD) and then add the corresponding
find_package() and target_link_libraries(imgui_static ...) entries (or add
find_package(GLEW) and link GLEW) to ensure the declared loader macro matches an
available dependency.
Comment on lines
+98
to
+105
| if (ImGui::Begin("##img_stub")) | ||
| { | ||
| ImGui::TextDisabled("(Image handler: TODO — upload CImage to GL texture)"); | ||
| if (auto obs = std::dynamic_pointer_cast<mrpt::obs::CObservation>(o); obs) | ||
| show_common_sensor_info(*obs, "image"); | ||
| } | ||
| ImGui::End(); | ||
| } |
There was a problem hiding this comment.
Hardcoded window ID will cause conflicts with multiple image sources.
The ImGui window ID "##img_stub" and the "image" key passed to show_common_sensor_info are hardcoded. If multiple CObservationImage or CObservation3DRangeScan observations are rendered, they will share the same ImGui window and rate estimation state, causing incorrect behavior.
Consider deriving a unique ID from the observation's sensorLabel or the subWindowTitle parameter (which the handler currently ignores).
💡 Suggested approach
The handler signature provides parentWin which could be combined with obj->sensorLabel to create unique keys:
// Extract sensor label for unique identification:
auto obs = std::dynamic_pointer_cast<mrpt::obs::CObservation>(o);
if (!obs) return;
const std::string winId = "##img_" + obs->sensorLabel;
if (ImGui::Begin(winId.c_str()))
{
show_common_sensor_info(*obs, obs->sensorLabel);
// ...
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mola_viz_imgui/src/MolaVizImGui_handlers.cpp` around lines 98 - 105, The
ImGui window ID and sensor key are hardcoded causing collisions; update the
handler that renders observations (uses ImGui::Begin and
show_common_sensor_info) to derive a unique ID/key from the observation's
sensorLabel (cast via std::dynamic_pointer_cast<mrpt::obs::CObservation>) or the
provided subWindowTitle/parentWin instead of using "##img_stub" and "image";
build a unique string (e.g., "##img_"+obs->sensorLabel) and pass
obs->sensorLabel (or the composed unique key) into show_common_sensor_info so
multiple CObservationImage / CObservation3DRangeScan instances maintain separate
ImGui windows and rate state.
Comment on lines
+92
to
+97
| MRPT_INITIALIZER(do_register_MolaVizImGui) // NOLINT | ||
| { | ||
| MOLA_REGISTER_MODULE(MolaVizImGui); | ||
| // TODO: register default sensor-observation GUI handlers (see | ||
| // MolaVizImGui_handlers.cpp once implemented). | ||
| } |
There was a problem hiding this comment.
Missing call to mola_viz_imgui_register_default_handlers().
The TODO comment indicates that default sensor-observation GUI handlers should be registered here, but the call to mola_viz_imgui_register_default_handlers() (defined in MolaVizImGui_handlers.cpp) is missing. Without this, none of the handlers (GPS, IMU, images, point clouds) will be active.
🐛 Proposed fix
MRPT_INITIALIZER(do_register_MolaVizImGui) // NOLINT
{
MOLA_REGISTER_MODULE(MolaVizImGui);
- // TODO: register default sensor-observation GUI handlers (see
- // MolaVizImGui_handlers.cpp once implemented).
+ // Register default sensor-observation GUI handlers:
+ mola_viz_imgui_register_default_handlers();
}You'll also need to declare the function in the header or at the top of this file:
// Forward declaration (or add to header):
void mola_viz_imgui_register_default_handlers();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mola_viz_imgui/src/MolaVizImGui.cpp` around lines 92 - 97, The MRPT
initializer do_register_MolaVizImGui registers the module but never registers
the default sensor-observation GUI handlers; add a forward declaration for void
mola_viz_imgui_register_default_handlers() near the top of this file (or in the
header) and call mola_viz_imgui_register_default_handlers() inside
MRPT_INITIALIZER(do_register_MolaVizImGui) after
MOLA_REGISTER_MODULE(MolaVizImGui) so GPS/IMU/image/point-cloud handlers are
activated.
Comment on lines
+845
to
+866
| std::future<bool> MolaVizImGui::subwindow_update_visualization( | ||
| const mrpt::rtti::CObject::Ptr& obj, const std::string& subWindowTitle, | ||
| const mrpt::containers::yaml* extra_parameters, const std::string& parentWindow) | ||
| { | ||
| auto task = std::make_shared<std::packaged_task<bool()>>( | ||
| [this, obj, subWindowTitle, extra_parameters, parentWindow]() | ||
| { | ||
| const char* cls = obj->GetRuntimeClass()->className; | ||
| auto& hc = ImGuiHandlersContainer::Instance(); | ||
| std::lock_guard lk(hc.mtx); | ||
| bool any = false; | ||
| for (auto [it, end] = hc.handlers.equal_range(cls); it != end; ++it) | ||
| { | ||
| it->second(obj, nullptr /*no raw ptr*/, parentWindow, this, extra_parameters); | ||
| any = true; | ||
| } | ||
| return any; | ||
| }); | ||
| std::lock_guard lk(guiThreadPendingTasksMtx_); | ||
| guiThreadPendingTasks_.emplace_back([=]() { (*task)(); }); | ||
| return task->get_future(); | ||
| } |
There was a problem hiding this comment.
Potential dangling pointer: extra_parameters captured by value.
The lambda captures extra_parameters (a raw pointer) by value. If the caller's mrpt::containers::yaml object is destroyed before the task executes on the GUI thread, this becomes a dangling pointer.
Consider either:
- Copying the yaml object if non-null, or
- Documenting that the yaml must outlive the async call, or
- Using
std::shared_ptr<const mrpt::containers::yaml>in the API
💡 Suggested fix: copy the yaml if provided
std::future<bool> MolaVizImGui::subwindow_update_visualization(
const mrpt::rtti::CObject::Ptr& obj, const std::string& subWindowTitle,
const mrpt::containers::yaml* extra_parameters, const std::string& parentWindow)
{
+ // Copy extra_parameters to avoid dangling pointer in async task:
+ auto extra_copy = extra_parameters
+ ? std::make_shared<mrpt::containers::yaml>(*extra_parameters)
+ : nullptr;
+
auto task = std::make_shared<std::packaged_task<bool()>>(
- [this, obj, subWindowTitle, extra_parameters, parentWindow]()
+ [this, obj, subWindowTitle, extra_copy, parentWindow]()
{
const char* cls = obj->GetRuntimeClass()->className;
auto& hc = ImGuiHandlersContainer::Instance();
std::lock_guard lk(hc.mtx);
bool any = false;
for (auto [it, end] = hc.handlers.equal_range(cls); it != end; ++it)
{
- it->second(obj, nullptr /*no raw ptr*/, parentWindow, this, extra_parameters);
+ it->second(obj, nullptr /*no raw ptr*/, parentWindow, this, extra_copy.get());
any = true;
}
return any;
});📝 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
| std::future<bool> MolaVizImGui::subwindow_update_visualization( | |
| const mrpt::rtti::CObject::Ptr& obj, const std::string& subWindowTitle, | |
| const mrpt::containers::yaml* extra_parameters, const std::string& parentWindow) | |
| { | |
| auto task = std::make_shared<std::packaged_task<bool()>>( | |
| [this, obj, subWindowTitle, extra_parameters, parentWindow]() | |
| { | |
| const char* cls = obj->GetRuntimeClass()->className; | |
| auto& hc = ImGuiHandlersContainer::Instance(); | |
| std::lock_guard lk(hc.mtx); | |
| bool any = false; | |
| for (auto [it, end] = hc.handlers.equal_range(cls); it != end; ++it) | |
| { | |
| it->second(obj, nullptr /*no raw ptr*/, parentWindow, this, extra_parameters); | |
| any = true; | |
| } | |
| return any; | |
| }); | |
| std::lock_guard lk(guiThreadPendingTasksMtx_); | |
| guiThreadPendingTasks_.emplace_back([=]() { (*task)(); }); | |
| return task->get_future(); | |
| } | |
| std::future<bool> MolaVizImGui::subwindow_update_visualization( | |
| const mrpt::rtti::CObject::Ptr& obj, const std::string& subWindowTitle, | |
| const mrpt::containers::yaml* extra_parameters, const std::string& parentWindow) | |
| { | |
| // Copy extra_parameters to avoid dangling pointer in async task: | |
| auto extra_copy = extra_parameters | |
| ? std::make_shared<mrpt::containers::yaml>(*extra_parameters) | |
| : nullptr; | |
| auto task = std::make_shared<std::packaged_task<bool()>>( | |
| [this, obj, subWindowTitle, extra_copy, parentWindow]() | |
| { | |
| const char* cls = obj->GetRuntimeClass()->className; | |
| auto& hc = ImGuiHandlersContainer::Instance(); | |
| std::lock_guard lk(hc.mtx); | |
| bool any = false; | |
| for (auto [it, end] = hc.handlers.equal_range(cls); it != end; ++it) | |
| { | |
| it->second(obj, nullptr /*no raw ptr*/, parentWindow, this, extra_copy.get()); | |
| any = true; | |
| } | |
| return any; | |
| }); | |
| std::lock_guard lk(guiThreadPendingTasksMtx_); | |
| guiThreadPendingTasks_.emplace_back([=]() { (*task)(); }); | |
| return task->get_future(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mola_viz_imgui/src/MolaVizImGui.cpp` around lines 845 - 866,
subwindow_update_visualization captures the raw pointer extra_parameters into a
packaged_task which may run later, creating a dangling pointer risk; fix by
making a safe copy of the yaml before creating the lambda (e.g., if
extra_parameters != nullptr create a std::shared_ptr<const
mrpt::containers::yaml> or a value copy and capture that instead) and then
capture the new shared_ptr/value in the packaged_task lambda used when pushing
into guiThreadPendingTasks_, ensuring the lambda no longer holds a raw pointer
to external memory.
mola_viz/src/MolaViz.cpp
Outdated
Comment on lines
+1930
to
+1931
| "Exception in execute_custom_code_on_background_scene():\n" | ||
| << e.what()); |
There was a problem hiding this comment.
Fix clang-format mismatch in this log statement.
CI is already failing on formatting in this block, so this needs reformatting before merge.
🎯 Suggested formatting fix
- MRPT_LOG_ERROR_STREAM(
- "Exception in execute_custom_code_on_background_scene():\n"
- << e.what());
+ MRPT_LOG_ERROR_STREAM("Exception in execute_custom_code_on_background_scene():\n"
+ << e.what());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mola_viz/src/MolaViz.cpp` around lines 1930 - 1931, The log stream in the
exception handler for execute_custom_code_on_background_scene() is misformatted
for clang-format; reflow the std::cerr/std::ostringstream (or the insertion
chain shown) into a single properly indented statement so the message and
e.what() are on the same line and the closing parentheses/semicolon align with
surrounding code style (e.g., combine the string literal and e.what() insertion
into one continuous insertion expression). Locate the exception logging code
that contains "Exception in execute_custom_code_on_background_scene():\n" and
adjust spacing/line breaks to match the project's clang-format rules.
jlblancoc
force-pushed
the
feat/dear-imgui-viz
branch
from
52d78b2 to
7922e20
Compare
jlblancoc
force-pushed
the
feat/dear-imgui-viz
branch
from
7922e20 to
ee5554e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
mola_viz_imgui/src/MolaVizImGui.cpp (2)
824-832: Considerstd::dequefor O(1) removal from front.Using
std::vector::erase(begin())is O(n) per removal. While acceptable for the defaultmax_console_lines_of 12, astd::dequewould provide O(1)pop_front()if console capacity is ever increased.💡 Optional: Use deque for console messages
In header (PerWindowData):
- std::vector<std::string> console_messages; + std::deque<std::string> console_messages;In cpp:
for (const auto& line : lines) { - wd.console_messages.push_back(line); - while (wd.console_messages.size() > max_console_lines_) - { - wd.console_messages.erase(wd.console_messages.begin()); - } + wd.console_messages.push_back(line); + while (wd.console_messages.size() > max_console_lines_) + wd.console_messages.pop_front(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mola_viz_imgui/src/MolaVizImGui.cpp` around lines 824 - 832, wd.console_messages is using std::vector and erasing from begin() inside the loop which is O(n); change the container to std::deque to get O(1) front removals: update the PerWindowData declaration of console_messages to std::deque<std::string>, keep using push_back(message) in MolaVizImGui.cpp, and replace the erase(begin()) loop with a simple while (wd.console_messages.size() > max_console_lines_) wd.console_messages.pop_front(); to maintain capacity.
394-407: Background scene rendering is not implemented.The 3D scene API methods (
update_3d_object,insert_point_cloud_with_decay, etc.) populatebackground_scenebutrender_background_scene()is a no-op stub. Users calling these APIs won't see any 3D content until this is implemented.Would you like me to help scaffold the FBO-based rendering implementation? The typical pattern involves:
- Create/resize FBO matching the dockspace central node dimensions
- Bind FBO and call
mrpt::opengl::Scene::render()with the camera parameters fromPerWindowData- Display the FBO texture via
ImGui::Image()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mola_viz_imgui/src/MolaVizImGui.cpp` around lines 394 - 407, render_background_scene is currently a stub so background_scene updates never get shown; implement FBO-based rendering: in MolaVizImGui::render_background_scene(PerWindowData& wd) create or resize an OpenGL FBO/texture to match the central dock node size each frame, bind the FBO, call the mrpt renderer (e.g. mrpt::opengl::Scene::render or equivalent) with wd.background_scene and the camera parameters stored on PerWindowData, unbind the FBO, then call ImGui::Image(reinterpret_cast<ImTextureID>(fbo_texture), node_size) to display the texture; ensure proper GL state cleanup and reuse the FBO across frames to avoid reallocating every frame and handle high-DPI scaling when computing node_size.mola_viz_imgui/include/mola_viz_imgui/MolaVizImGui.h (1)
240-245: Consider atomic types for thread-safe parameter access.These parameters are set in
initialize()(non-GUI thread) and read ingui_thread()and rendering helpers. While the current usage pattern (write-once, then read-only) is safe in practice, making themstd::atomicwould provide explicit memory ordering guarantees.💡 Optional: Use atomics for explicit thread safety
- double console_text_font_size_ = 13.0; - unsigned int max_console_lines_ = 12; - bool show_rgbd_as_point_cloud_ = false; - double assumed_sensor_rate_hz_ = 10.0; - int target_fps_ = 60; + std::atomic<double> console_text_font_size_{13.0}; + std::atomic<unsigned int> max_console_lines_{12}; + std::atomic<bool> show_rgbd_as_point_cloud_{false}; + std::atomic<double> assumed_sensor_rate_hz_{10.0}; + std::atomic<int> target_fps_{60};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mola_viz_imgui/include/mola_viz_imgui/MolaVizImGui.h` around lines 240 - 245, Change the non-thread-safe primitive parameters to atomic types so writes in initialize() and reads in gui_thread()/render helpers have explicit memory ordering: replace console_text_font_size_ (double) and assumed_sensor_rate_hz_ (double) with std::atomic<double>, max_console_lines_ (unsigned int) with std::atomic<unsigned int>, target_fps_ (int) with std::atomic<int>, and show_rgbd_as_point_cloud_ (bool) with std::atomic<bool>; include <atomic>, update assignments in initialize() to use .store(...) and update reads in gui_thread() and any rendering helpers to use .load() (or the atomic operators) to ensure safe cross-thread access while preserving the current write-once/read-many semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mola_viz_imgui/src/MolaVizImGui.cpp`:
- Line 214: The code disables ImGui layout persistence by setting io.IniFilename
= nullptr; to fix this, assign io.IniFilename to a persistent C-string derived
from the window/name-based ini filename (e.g., "<name>.imgui.ini") instead of
nullptr; store these strings in a static map or container that outlives ImGui
(so the C-string pointer remains valid) and set io.IniFilename from that map
during your ImGui initialization (the spot where io.IniFilename is currently
set), ensuring the docking/layout persistence described in MolaVizImGui.h works
as documented.
---
Nitpick comments:
In `@mola_viz_imgui/include/mola_viz_imgui/MolaVizImGui.h`:
- Around line 240-245: Change the non-thread-safe primitive parameters to atomic
types so writes in initialize() and reads in gui_thread()/render helpers have
explicit memory ordering: replace console_text_font_size_ (double) and
assumed_sensor_rate_hz_ (double) with std::atomic<double>, max_console_lines_
(unsigned int) with std::atomic<unsigned int>, target_fps_ (int) with
std::atomic<int>, and show_rgbd_as_point_cloud_ (bool) with std::atomic<bool>;
include <atomic>, update assignments in initialize() to use .store(...) and
update reads in gui_thread() and any rendering helpers to use .load() (or the
atomic operators) to ensure safe cross-thread access while preserving the
current write-once/read-many semantics.
In `@mola_viz_imgui/src/MolaVizImGui.cpp`:
- Around line 824-832: wd.console_messages is using std::vector and erasing from
begin() inside the loop which is O(n); change the container to std::deque to get
O(1) front removals: update the PerWindowData declaration of console_messages to
std::deque<std::string>, keep using push_back(message) in MolaVizImGui.cpp, and
replace the erase(begin()) loop with a simple while (wd.console_messages.size()
> max_console_lines_) wd.console_messages.pop_front(); to maintain capacity.
- Around line 394-407: render_background_scene is currently a stub so
background_scene updates never get shown; implement FBO-based rendering: in
MolaVizImGui::render_background_scene(PerWindowData& wd) create or resize an
OpenGL FBO/texture to match the central dock node size each frame, bind the FBO,
call the mrpt renderer (e.g. mrpt::opengl::Scene::render or equivalent) with
wd.background_scene and the camera parameters stored on PerWindowData, unbind
the FBO, then call ImGui::Image(reinterpret_cast<ImTextureID>(fbo_texture),
node_size) to display the texture; ensure proper GL state cleanup and reuse the
FBO across frames to avoid reallocating every frame and handle high-DPI scaling
when computing node_size.
🪄 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: 8771adb6-158b-4890-b9b5-00321614f9e9
📒 Files selected for processing (15)
.gitmodulesmola_demos/mola-cli-launchs/kitti360_just_replay.yamlmola_demos/mola-cli-launchs/kitti_just_replay.yamlmola_viz/README.mdmola_viz/src/MolaViz.cppmola_viz_imgui/3rdparty/.clang-formatmola_viz_imgui/3rdparty/imguimola_viz_imgui/3rdparty/imgui_static/CMakeLists.txtmola_viz_imgui/CMakeLists.txtmola_viz_imgui/README.mdmola_viz_imgui/include/mola_viz_imgui/MolaVizImGui.hmola_viz_imgui/package.xmlmola_viz_imgui/src/MolaVizImGui.cppmola_viz_imgui/src/MolaVizImGui_handlers.cppmola_viz_imgui/src/MolaVizImGui_widgets.cpp
✅ Files skipped from review due to trivial changes (10)
- mola_viz/README.md
- mola_viz_imgui/3rdparty/.clang-format
- .gitmodules
- mola_viz_imgui/3rdparty/imgui
- mola_viz/src/MolaViz.cpp
- mola_viz_imgui/README.md
- mola_demos/mola-cli-launchs/kitti360_just_replay.yaml
- mola_viz_imgui/package.xml
- mola_viz_imgui/CMakeLists.txt
- mola_viz_imgui/src/MolaVizImGui_widgets.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- mola_demos/mola-cli-launchs/kitti_just_replay.yaml
- mola_viz_imgui/src/MolaVizImGui_handlers.cpp
| ImGuiIO& io = ImGui::GetIO(); | ||
| io.ConfigFlags |= ImGuiConfigFlags_DockingEnable; // docking branch | ||
| io.ConfigFlags |= ImGuiConfigFlags_NavEnableKeyboard; | ||
| io.IniFilename = nullptr; // TODO: set per-window ini path from name |
There was a problem hiding this comment.
ImGui layout persistence is disabled.
Setting io.IniFilename = nullptr prevents ImGui from saving/loading imgui.ini, which contradicts the documented behavior in MolaVizImGui.h (line 59-60): "the ImGui docking system handles placement and persistence via imgui.ini automatically."
Without this, users lose their docking layouts on restart. Consider enabling it:
💡 Suggested fix: Enable ini persistence per window
- io.IniFilename = nullptr; // TODO: set per-window ini path from name
+ // Enable per-window ini file for docking persistence:
+ static std::map<std::string, std::string> iniFilenames;
+ iniFilenames[name] = "imgui_mola_" + name + ".ini";
+ io.IniFilename = iniFilenames[name].c_str();The static map ensures the string outlives the ImGuiIO reference.
📝 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
| io.IniFilename = nullptr; // TODO: set per-window ini path from name | |
| // Enable per-window ini file for docking persistence: | |
| static std::map<std::string, std::string> iniFilenames; | |
| iniFilenames[name] = "imgui_mola_" + name + ".ini"; | |
| io.IniFilename = iniFilenames[name].c_str(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mola_viz_imgui/src/MolaVizImGui.cpp` at line 214, The code disables ImGui
layout persistence by setting io.IniFilename = nullptr; to fix this, assign
io.IniFilename to a persistent C-string derived from the window/name-based ini
filename (e.g., "<name>.imgui.ini") instead of nullptr; store these strings in a
static map or container that outlives ImGui (so the C-string pointer remains
valid) and set io.IniFilename from that map during your ImGui initialization
(the spot where io.IniFilename is currently set), ensuring the docking/layout
persistence described in MolaVizImGui.h works as documented.
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 by CodeRabbit
New Features
Documentation
Chores