Add get function for window menu object #7295

EwingKang · 2025-07-01T02:57:59Z

Useful for changing submenu/ items in the menu

Provide a get function for window menu object.
Provide a get function for menubar in O3DVisualizer

Type

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Motivation and Context

The current implementation doesn't provide means to retrieve submenu/ items. Which makes it impossible to add menu items to the existing O3DVisualizer

Checklist:

I have run ~~python util/check_style.py --apply~~ make check-style && make apply-style to apply Open3D code style
to my code.
This PR changes Open3D behavior or adds new functionality.
- Both C++ (Doxygen) and Python (Sphinx / Google style) documentation is
  updated accordingly.
- I have added or updated C++ and / or Python unit tests OR included test
  results (e.g. screenshots or numbers) here.
I will follow up and update the code if CI fails.
For fork PRs, I have selected Allow edits from maintainers.

Description

This commit makes it possible to add item to the existing menu in the visualizer.

For example, we can easily add an menu item with proper callback behavior (file loading in this example) like this:

using namespace open3d::visualization
void OnMenuLoadCb(visualizer::O3DVisualizer* window)
{
	LOG_INFO("Menu CB!");

	auto dlg = std::make_shared<gui::FileDialog>(
			gui::FileDialog::Mode::OPEN, "Open File", window->GetTheme());
	dlg->AddFilter(".obj", "OBJ meshes for spray planning (.obj)");
	dlg->AddFilter("", "All files");
	dlg->SetOnCancel([window]() { window->CloseDialog(); });
	dlg->SetOnDone([window](const char *path) {
		window->CloseDialog();
		LOG_INFO("Loading file: " + std::string(path) );
	});
	window->ShowDialog(dlg);
}

int main()
{
	auto o3dvis = std::make_shared<visualizer::O3DVisualizer>( "Visualize", _width, height );
	//...
	o3dvis->GetMenubar()->GetMenu("File")->InsertItem(1, "Load STPF file", 599);
	o3dvis->SetOnMenuItemActivated(599, [o3dvis](){OnMenuLoadCb(o3dvis.get());});
}

With the result:

This change is

Usefull for changing submenu/ items in the menu * Provide get function for window menu object. * Provide get function for menubar in O3DVisualizer

update-docs · 2025-07-01T02:58:04Z

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

Copilot

Pull Request Overview

This PR adds retrieval functions for the menu bar and submenus, enabling users to query and modify existing menus in O3DVisualizer.

Adds GetMenubar to O3DVisualizer for access to the application menubar
Introduces GetMenu(name) and GetMenu(item_id) in MenuBase and implements them in Menu and MenuImgui
Updates changelog to document the new menu APIs

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
cpp/open3d/visualization/visualizer/O3DVisualizer.h	Declares `GetMenubar` on `O3DVisualizer`
cpp/open3d/visualization/visualizer/O3DVisualizer.cpp	Implements both impl‐ and wrapper versions of `GetMenubar`
cpp/open3d/visualization/gui/MenuBase.h	Adds pure virtual `GetMenu` overloads
cpp/open3d/visualization/gui/MenuImgui.h & MenuImgui.cpp	Declares and defines `GetMenu` in ImGui‐based menu
cpp/open3d/visualization/gui/Menu.h & Menu.cpp	Declares and defines `GetMenu` in core `Menu`
CHANGELOG.md	Documents the new menu‐retrieval feature

Comments suppressed due to low confidence (4)

cpp/open3d/visualization/visualizer/O3DVisualizer.h:166

[nitpick] Consider renaming GetMenubar to GetMenuBar (capital ‘B’) to follow standard camel‐case conventions for multiword identifiers, and mark it as a const method.

    std::shared_ptr<gui::MenuBase> GetMenubar();

cpp/open3d/visualization/visualizer/O3DVisualizer.h:164

[nitpick] The doc comment is a bit unclear. You could simplify to: /// Returns a shared_ptr to the application's menubar for this visualizer window.

    /// Get menubar from window system. The window came from

cpp/open3d/visualization/visualizer/O3DVisualizer.h:166

There are no existing tests for the new GetMenubar method—consider adding unit tests to verify it returns the correct menubar.

    std::shared_ptr<gui::MenuBase> GetMenubar();

CHANGELOG.md:63

[nitpick] Changelog mentions GetMenu on MenuBase but omits the new GetMenubar API—consider updating to include both additions.

-   Add `GetMenu` for MenuBase for easy menu item/submenu control. (PR #7295)

Copilot · 2025-07-01T20:19:12Z

cpp/open3d/visualization/visualizer/O3DVisualizer.cpp

@@ -1400,6 +1400,10 @@ Ctrl-alt-click to polygon select)";
        scene_->ForceRedraw();
    }

+    std::shared_ptr<MenuBase> GetMenubar() {


Qualify MenuBase with its namespace (e.g., gui::MenuBase) to avoid ambiguity and match the header declaration.

Suggested change

std::shared_ptr<MenuBase> GetMenubar() {

std::shared_ptr<gui::MenuBase> GetMenubar() {

I'm open to do it, however, the same file L:45 already have a similar case without gui:: namespace. I followed the convention. Which is better?
https://github.com/EwingKang/Open3D/blob/menu_get_fun/cpp/open3d/visualization/gui/MenuBase.h#L47

Copilot · 2025-07-01T20:19:13Z

cpp/open3d/visualization/visualizer/O3DVisualizer.cpp

@@ -2427,6 +2431,10 @@ void O3DVisualizer::ModifyGeometryMaterial(
    impl_->ModifyGeometryMaterial(name, material);
 }

+std::shared_ptr<MenuBase> O3DVisualizer::GetMenubar() {


This should be marked const since it does not modify object state: std::shared_ptr<gui::MenuBase> O3DVisualizer::GetMenubar() const {

I'm baffled. Does the modification of the smart-pointer-pointed count as mutation to the object? I'm open to both ways.

Personally, after reading https://isocpp.org/wiki/faq/const-correctness#logical-vs-physical-const and some stackoverflow articles, I would say that this member should NOT be marked as const. Since the const label hints the programmer that no part of this object will be changed. This would not be intuitive since the purpose of the get function is for the user to modify the content. For example the sample code provided in the original PR.

To be more specific, almost all discussion I read have the following syntax:

int *GetPointerToMember() { return &member; } const int *GetPointerToMember() const { return &member; }

Usually there are always two versions of the get function. Should I straight up provide two versions of the get function?
@ssheorey I hate to bother you with this trivial question, but it would be nice if you can give some pointers.

Copilot · 2025-07-01T20:19:13Z

cpp/open3d/visualization/gui/MenuBase.h

+    virtual std::shared_ptr<MenuBase> GetMenu(const char* name) = 0;
+
+    /// Get the Menu by ItemId. Returns nullptr if not found.
+    virtual std::shared_ptr<MenuBase> GetMenu(ItemId item_id) = 0;


Both GetMenu overloads should be marked const, e.g., virtual std::shared_ptr<MenuBase> GetMenu(const char* name) const = 0;, to reflect that they do not modify the menu.

Suggested change

virtual std::shared_ptr<MenuBase> GetMenu(const char* name) = 0;

/// Get the Menu by ItemId. Returns nullptr if not found.

virtual std::shared_ptr<MenuBase> GetMenu(ItemId item_id) = 0;

virtual std::shared_ptr<MenuBase> GetMenu(const char* name) const = 0;

/// Get the Menu by ItemId. Returns nullptr if not found.

virtual std::shared_ptr<MenuBase> GetMenu(ItemId item_id) const = 0;

Copilot · 2025-07-01T20:19:14Z

cpp/open3d/visualization/gui/MenuImgui.h

@@ -35,6 +35,9 @@ class MenuImgui : public MenuBase {

    int GetNumberOfItems() const override;

+    std::shared_ptr<MenuBase> GetMenu(const char* name) override;


Add a const qualifier to this override: std::shared_ptr<MenuBase> GetMenu(const char* name) const override;

Suggested change

std::shared_ptr<MenuBase> GetMenu(const char* name) override;

std::shared_ptr<MenuBase> GetMenu(const char* name) const override;

Copilot · 2025-07-01T20:19:14Z

cpp/open3d/visualization/gui/Menu.h

@@ -45,6 +45,12 @@ class Menu : public MenuBase {

    int GetNumberOfItems() const override;

+    /// Get the Menu by name. Returns nullptr if not found.
+    std::shared_ptr<MenuBase> GetMenu(const char* name) override;


Similarly, mark this override as const: std::shared_ptr<MenuBase> GetMenu(const char* name) const override;

Suggested change

std::shared_ptr<MenuBase> GetMenu(const char* name) override;

std::shared_ptr<MenuBase> GetMenu(const char* name) const override;

EwingKang · 2025-07-02T09:26:41Z

Copilot suggests that I should use GetMenuBar instead of GetMenubar. How funny it is that this is actually my original naming. The reason I eventually changed it to GetMenubar is because there is already an existing example in https://github.com/EwingKang/Open3D/blob/menu_get_fun/cpp/open3d/visualization/gui/Application.h#L85, we have

Open3D/cpp/open3d/visualization/gui/Application.h

Lines 84 to 87 in 35beb51

    
           std::shared_ptr<Menu> GetMenubar() const; 
        
           void SetMenubar(std::shared_ptr<Menu> menubar);

Add get function for window menu object

1a093e7

Usefull for changing submenu/ items in the menu * Provide get function for window menu object. * Provide get function for menubar in O3DVisualizer

Modify CHANGELOG - Add PR isl-org#7295

1681173

ssheorey requested review from Copilot and ssheorey July 1, 2025 20:17

Copilot AI reviewed Jul 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add get function for window menu object #7295

Add get function for window menu object #7295

EwingKang commented Jul 1, 2025 •

edited

Loading

Uh oh!

update-docs bot commented Jul 1, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jul 1, 2025

Uh oh!

EwingKang Jul 2, 2025 •

edited

Loading

Uh oh!

Copilot AI Jul 1, 2025

Uh oh!

EwingKang Jul 2, 2025

Uh oh!

EwingKang Jul 2, 2025

Uh oh!

Copilot AI Jul 1, 2025

Uh oh!

Copilot AI Jul 1, 2025

Uh oh!

Copilot AI Jul 1, 2025

Uh oh!

EwingKang commented Jul 2, 2025 •

edited

Loading

Uh oh!

Uh oh!

	std::shared_ptr<MenuBase> GetMenubar() {
	std::shared_ptr<gui::MenuBase> GetMenubar() {

		@@ -35,6 +35,9 @@ class MenuImgui : public MenuBase {

		int GetNumberOfItems() const override;

		std::shared_ptr<MenuBase> GetMenu(const char* name) override;

Add get function for window menu object #7295

Are you sure you want to change the base?

Add get function for window menu object #7295

Conversation

EwingKang commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type

Motivation and Context

Checklist:

Description

Uh oh!

update-docs bot commented Jul 1, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

EwingKang Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

EwingKang Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

EwingKang Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

EwingKang commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

EwingKang commented Jul 1, 2025 •

edited

Loading

EwingKang Jul 2, 2025 •

edited

Loading

EwingKang commented Jul 2, 2025 •

edited

Loading