mosra / magnum-integration

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

Add support for draw list user callbacks in ImGui integration #84

Closed arximboldi closed 2 years ago

arximboldi commented 3 years ago

Fixes #83

codecov[bot] commented 3 years ago

Codecov Report

Merging #84 (3b64a76) into master (8e1ee6c) will decrease coverage by 13.37%. The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #84       +/-   ##
===========================================
- Coverage   89.83%   76.45%   -13.38%     
===========================================
  Files          16       21        +5     
  Lines         590      960      +370     
===========================================
+ Hits          530      734      +204     
- Misses         60      226      +166     
Impacted Files Coverage Δ
src/Magnum/ImGuiIntegration/Context.cpp 85.97% <66.66%> (-4.94%) :arrow_down:
src/Magnum/EigenIntegration/Integration.h 100.00% <0.00%> (ø)
src/Magnum/DartIntegration/ConvertShapeNode.h 100.00% <0.00%> (ø)
src/Magnum/DartIntegration/Object.cpp 0.00% <0.00%> (ø)
src/Magnum/DartIntegration/Object.h 0.00% <0.00%> (ø)
src/Magnum/DartIntegration/ConvertShapeNode.cpp 96.48% <0.00%> (ø)
src/Magnum/DartIntegration/World.cpp 0.00% <0.00%> (ø)
src/Magnum/BulletIntegration/MotionState.cpp 85.71% <0.00%> (+1.50%) :arrow_up:
src/Magnum/BulletIntegration/DebugDraw.cpp 15.90% <0.00%> (+2.02%) :arrow_up:
... and 1 more

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 8e1ee6c...3b64a76. Read the comment docs.

pezcode commented 2 years ago

Hey @arximboldi 👋 Any chance you could rebase on top of master and fix the code style to match the Corrade coding style?

Namely:

Ideally there would also be a test in ContextGLTest.cpp that checks that the callback is called correctly.

Let me know if you don't have the time or want me to do this, I don't mind taking over.

mosra commented 2 years ago

This again slipped through my attention, sorry. At first I waited until we had the test scaffolding set up, but then just completely forgot to get back to this.

@pezcode if you're able to add the tests, that would be the most great. Coding-style-wise, don't bother, I can fix that during merge ;)

arximboldi commented 2 years ago

I don't have time to work on this now so I would appreciate if someone can pick it up. Thank you a lot!

pezcode commented 2 years ago

I opened https://github.com/mosra/magnum-integration/pull/98. I would've preferred to have just one PR, but this way we get CI tests running.