Skip to content

Add get function for window menu object #7295

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add get function for window menu object #7295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a093e7
Add get function for window menu object
Jul 1, 2025
1681173
Modify CHANGELOG - Add PR #7295
Jul 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
- Fix CMake configuration summary incorrectly reporting `no` for system BLAS. (PR #7230)
- Add error handling for insufficient correspondences in AdvancedMatching (PR #7234)
- Exposed `get_plotly_fig` and modified `draw_plotly` to return the `Figure` it creates. (PR #7258)
- Add `GetMenu` for MenuBase for easy menu item/submenu control. (PR #7295)

## 0.13

Expand Down
8 changes: 8 additions & 0 deletions cpp/open3d/visualization/gui/Menu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,14 @@ void Menu::InsertSeparator(int index) { impl_->menu->InsertSeparator(index); }

int Menu::GetNumberOfItems() const { return impl_->menu->GetNumberOfItems(); }

std::shared_ptr<MenuBase> Menu::GetMenu(const char* name) {
return impl_->menu->GetMenu(name);
}

std::shared_ptr<MenuBase> Menu::GetMenu(ItemId item_id) {
return impl_->menu->GetMenu(item_id);
}

bool Menu::IsEnabled(ItemId item_id) const {
return impl_->menu->IsEnabled(item_id);
}
Expand Down
6 changes: 6 additions & 0 deletions cpp/open3d/visualization/gui/Menu.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copilot uses AI. Check for mistakes.

/// Get the Menu by ItemId. Returns nullptr if not found.
std::shared_ptr<MenuBase> GetMenu(ItemId item_id) override;

/// Searches the menu hierarchy down from this menu to find the item
/// and returns true if the item is enabled.
bool IsEnabled(ItemId item_id) const override;
Expand Down
6 changes: 6 additions & 0 deletions cpp/open3d/visualization/gui/MenuBase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@ class MenuBase {

virtual int GetNumberOfItems() const = 0;

/// Get the Menu by name. Returns nullptr if not found.
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;
Comment on lines +53 to +56
Copy link

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.

/// Searches the menu hierarchy down from this menu to find the item
/// and returns true if the item is enabled.
virtual bool IsEnabled(ItemId item_id) const = 0;
Expand Down
17 changes: 17 additions & 0 deletions cpp/open3d/visualization/gui/MenuImgui.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,23 @@ void MenuImgui::InsertSeparator(int index) {

int MenuImgui::GetNumberOfItems() const { return int(impl_->items_.size()); }

std::shared_ptr<MenuBase> MenuImgui::GetMenu(const char *name) {
for (auto &item : impl_->items_) {
if (item.name_.compare(name) == 0) {
return item.submenu_;
}
}
return nullptr;
}

std::shared_ptr<MenuBase> MenuImgui::GetMenu(ItemId item_id) {
auto item = impl_->FindMenuItem(item_id);
if (item) {
return item->submenu_;
}
return nullptr;
}

bool MenuImgui::IsEnabled(ItemId item_id) const {
auto *item = impl_->FindMenuItem(item_id);
if (item) {
Expand Down
3 changes: 3 additions & 0 deletions cpp/open3d/visualization/gui/MenuImgui.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ class MenuImgui : public MenuBase {

int GetNumberOfItems() const override;

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

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
std::shared_ptr<MenuBase> GetMenu(ItemId item_id) override;

bool IsEnabled(ItemId item_id) const override;
void SetEnabled(ItemId item_id, bool enabled) override;

Expand Down
8 changes: 8 additions & 0 deletions cpp/open3d/visualization/visualizer/O3DVisualizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1400,6 +1400,10 @@ Ctrl-alt-click to polygon select)";
scene_->ForceRedraw();
}

std::shared_ptr<MenuBase> GetMenubar() {
Copy link

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() {

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@EwingKang EwingKang Jul 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return Application::GetInstance().GetMenubar();
}

void ShowSettings(bool show, bool cancel_auto_show = true) {
if (cancel_auto_show) {
can_auto_show_settings_ = false;
Expand Down Expand Up @@ -2427,6 +2431,10 @@ void O3DVisualizer::ModifyGeometryMaterial(
impl_->ModifyGeometryMaterial(name, material);
}

std::shared_ptr<MenuBase> O3DVisualizer::GetMenubar() {
Copy link

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return impl_->GetMenubar();
}

void O3DVisualizer::ShowSettings(bool show) { impl_->ShowSettings(show); }

void O3DVisualizer::ShowSkybox(bool show) { impl_->ShowSkybox(show); }
Expand Down
4 changes: 4 additions & 0 deletions cpp/open3d/visualization/visualizer/O3DVisualizer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,10 @@ class O3DVisualizer : public gui::Window {

void ResetCameraToDefault();

/// Get menubar from window system. The window came from
/// `Application::GetInstance()`
std::shared_ptr<gui::MenuBase> GetMenubar();

void ShowSettings(bool show);
void ShowSkybox(bool show);
void SetIBL(const std::string& path);
Expand Down
Loading