mosra / magnum-integration

Integration libraries for the Magnum C++11 graphics engine
https://magnum.graphics/
Other
97 stars 44 forks source link

ImGuiIntegration: Allow to include another imconfig #85

Closed gergondet closed 2 years ago

gergondet commented 3 years ago

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[bot] commented 3 years ago

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 commented 3 years ago

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! :+1:

gergondet commented 3 years ago

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 commented 2 years ago

Sorry for the extremely delayed responses :see_no_evil:

Thanks for the docs update :+1: Merged this as 1faf8d3e7c960ac57db80e90c48ce6aad19e86a1, 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!