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

ImGuiIntegration: Allow to include another imconfig #85

Closed
wants to merge 1 commit into from

Conversation

gergondet
Copy link
Contributor

@gergondet gergondet commented Jul 16, 2021

The integration defines IMGUI_USER_CONFIG to change the visibility of ImGui API. If one sets IMGUI_USER_CONFIG in their own project this will create a conflict.

By defining MAGNUM_IMGUI_USER_CONFIG one can include their own file along the ImGuiIntegration provided one.

Note: maybe it would be more inline with the rest of the library to have the macro named MAGNUM_IMGUIINTEGRATION_USER_CONFIG, I have no problem with changing the name if you feel this is better

Thanks for taking time to consider this and for the great work on the overall project!

@codecov
Copy link

codecov bot commented Jul 16, 2021

Codecov Report

Merging #85 (5378ec1) into master (320f590) will increase coverage by 0.60%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
+ Coverage   76.25%   76.86%   +0.60%     
==========================================
  Files          21       21              
  Lines         956      981      +25     
==========================================
+ Hits          729      754      +25     
  Misses        227      227              
Impacted Files Coverage Δ
src/Magnum/EigenIntegration/GeometryIntegration.h 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 320f590...5378ec1. Read the comment docs.

@mosra mosra added this to the 2020.0b milestone Jul 25, 2021
@mosra mosra added this to TODO in GUI via automation Jul 25, 2021
@mosra
Copy link
Owner

mosra commented Jul 25, 2021

Hi, sorry for the delayed response, I have a constant notification overload :)

maybe it would be more inline with the rest of the library to have the macro named MAGNUM_IMGUIINTEGRATION_USER_CONFIG

Yes, that sounds better. Can you change it?

Another thing, it might be a good idea to document this somewhere, because otherwise I fear the feature will get forgotten. Can you add a paragraph about the macro and why IMGUI_USER_CONFIG doesn't work to this place? It would then appear here which I think is a good-enough location.

Thank you! 👍

The integration uses IMGUI_USER_CONFIG to change the visibility of imgui
API. If one sets IMGUI_USER_CONFIG in their own project this will create
a conflict.

By introducing MAGNUM_IMGUIINTEGRATION_USER_CONFIG one can include their own file
along the ImGuiIntegration provided one.
@gergondet
Copy link
Contributor Author

Hi @mosra

Thank you for the feedback and sorry for the delay on my side. I've taken you remarks into account. The macro has been renamed and I've added a small paragraph about the feature in the place you suggested.

Thanks!

@mosra
Copy link
Owner

mosra commented Sep 12, 2021

Sorry for the extremely delayed responses 🙈

Thanks for the docs update 👍 Merged this as 1faf8d3, for good measure I added a test as well -- was suspicious that the quotes in CMake could break on some systems, but apparently it works everywhere the CIs test for, so great!

@mosra mosra closed this Sep 12, 2021
GUI automation moved this from TODO to Done Sep 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
GUI
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants