search

mosra / magnum-integration

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

ImGuiIntegration: Update reference to Context::_texture after moving Context #62

Closed Squareys closed 4 years ago

Squareys commented 4 years ago

Hi @mosra !

As mentioned on #61, the move constructor needs to update the pointer to _texture, otherwise rendering breaks because it uses the then deinitialized texture memory.

Best, Jonathan

mosra commented 4 years ago

Oh and a test for this would be vital -- here https://github.com/mosra/magnum-integration/blob/d4f60dfc5c0aa257f50e960d2904495fb8ae6258/src/Magnum/ImGuiIntegration/Test/ContextGLTest.cpp#L262 it needs to check that &b.atlasTexture() == io.Fonts.TexID. And also &c.atlasTexture() == io.Fonts.TexID, which will notify you that you need to do a similar change in the move assignment as well.

codecov-io commented 4 years ago

Codecov Report

Merging #62 into master will increase coverage by 17.37%. The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #62       +/-   ##
===========================================
+ Coverage   74.13%   91.51%   +17.37%     
===========================================
  Files          21       16        -5     
  Lines         901      542      -359     
===========================================
- Hits          668      496      -172     
+ Misses        233       46      -187
Impacted Files Coverage Δ
src/Magnum/ImGuiIntegration/Context.cpp 94.96% <100%> (+11.73%) :arrow_up:
src/Magnum/GlmIntegration/Integration.cpp 92.85% <0%> (-3.58%) :arrow_down:
src/Magnum/BulletIntegration/MotionState.cpp 83.33% <0%> (-2.39%) :arrow_down:
src/Magnum/BulletIntegration/Integration.h 100% <0%> (ø) :arrow_up:
src/Magnum/ImGuiIntegration/Context.h 100% <0%> (ø) :arrow_up:
src/Magnum/DartIntegration/World.cpp
src/Magnum/DartIntegration/Object.cpp
src/Magnum/DartIntegration/ConvertShapeNode.h
src/Magnum/DartIntegration/ConvertShapeNode.cpp
src/Magnum/DartIntegration/Object.h
... and 2 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 d4f60df...681d349. Read the comment docs.

Squareys commented 4 years ago

@mosra I updated the PR to reflect suggested changes. Note that I am restoring the original imgui context, which is backwards compatible and also tested by the tests to be the case.

Squareys commented 4 years ago

Awesome, thanks! 🙏

rune-scape commented 4 years ago

I'm so embarrassed, i can't believe i missed this!

mosra commented 4 years ago

No problem, @Squareys missed a part of this also ... and I should be blamed for not noticing the issues in both PRs ;)