Skip to content
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.

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

Fixed warnings + added a new macro #1

Closed
wants to merge 4 commits into from

Conversation

BullyWiiPlaza
Copy link

@BullyWiiPlaza BullyWiiPlaza commented Nov 3, 2023

Hello,

thanks for your useful header.

I fixed compile warnings, including:

  • constexpr can be used
  • hides previous local declaration due to _ui_scope_guard being multiply defined in the same scope when using more than 1 of your macros

I also added a new macro with_Frame which might be useful to start a new ImGui frame. It supports injecting a platform specific rendering setup with the begin_frame function.

I just wanted to contribute something back in case you care, if not, that's fine, too.

@mnesarco
Copy link
Owner

mnesarco commented Nov 3, 2023

Thank you for your contribution. I will review it, but at a first bird eye there are a couple of things that are not subject to change here in the upstream version:

  • file names (imgui_sugar.hpp)
  • imgui subfolder inclusion (imgui/imgui.h)
  • coding style (code format)
  • a dependency on std will not be added (<functional>)

Other notes:

  • The constexpr if check is nice, good catch.
  • hiding names in deeper scopes is not an error but I can see that a warning could be annoying. I will check the issue and your solution (IMGUI_SUGAR_UNIQUE_NAME)

Once again, thank you for contributing back your ideas.

@mnesarco
Copy link
Owner

mnesarco commented Nov 3, 2023

The constexpr if is a c++17 feature. As the minimum supported version of imgui is c++14, this cannot be added yet.

@BullyWiiPlaza
Copy link
Author

The constexpr if is a c++17 feature. As the minimum supported version of imgui is c++14, this cannot be added yet.

I see. If someone is using the latest C++ standard in Visual Studio, this code generates a warning. Maybe it would be better to suppress it via some pragma then to keep it C++14 compatible.

mnesarco added a commit that referenced this pull request Nov 15, 2023
@mnesarco
Copy link
Owner

Hi @BullyWiiPlaza , thank you for your contribution. I have fixed the issue with a different trick. Now it is effectively doing the same thing as if constexpr (...) { ... } but without using it, so no hard requirement to require c++17.

@mnesarco
Copy link
Owner

IMGUI_SUGAR_UNIQUE_NAME is good. If you what to send a PR with only that change, I will merge then. Or if you prefer I can do the change directly.

@BullyWiiPlaza
Copy link
Author

IMGUI_SUGAR_UNIQUE_NAME is good. If you what to send a PR with only that change, I will merge then. Or if you prefer I can do the change directly.

You can go ahead and commit all changes you like by yourself, that's fine.

@mnesarco
Copy link
Owner

Summary:

Accepted and Included changes:

  1. Fix include to <imgui.h>
  2. manage if constexpr (without c++17)
  3. IMGUI_SUGAR_UNIQUE_NAME

Not included:

  1. File name changes
  2. Coding style changes
  3. requirement of c++17
  4. std dependency

@mnesarco mnesarco closed this Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants