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: Update reference to Context::_texture after moving Context #62

Merged
merged 1 commit into from Jan 5, 2020

Conversation

Squareys
Copy link
Contributor

@Squareys Squareys commented Jan 5, 2020

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

Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Somehow I didn't realize this was missing.

@@ -130,6 +130,8 @@ Context::Context(NoCreateT) noexcept: _context{nullptr}, _shader{NoCreate}, _tex

Context::Context(Context&& other) noexcept: _context{other._context}, _shader{std::move(other._shader)}, _texture{std::move(other._texture)}, _vertexBuffer{std::move(other._vertexBuffer)}, _indexBuffer{std::move(other._indexBuffer)}, _timeline{std::move(other._timeline)}, _mesh{std::move(other._mesh)}, _supersamplingRatio{other._supersamplingRatio}, _eventScaling{other._eventScaling} {
other._context = nullptr;
/* Update the pointer to _texture */
ImGui::GetIO().Fonts->SetTexID(reinterpret_cast<ImTextureID>(&_texture));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should do ImGui::SetCurrentContext(_context); first, to ensure we don't update some other context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then set it back later?

@mosra mosra added this to the 2020.0a milestone Jan 5, 2020
@mosra mosra added this to TODO in GUI via automation Jan 5, 2020
@mosra
Copy link
Owner

mosra commented Jan 5, 2020

Oh and a test for this would be vital -- here

CORRADE_COMPARE(ImGui::GetCurrentContext(), context);
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
Copy link

codecov-io commented Jan 5, 2020

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%) ⬆️
src/Magnum/GlmIntegration/Integration.cpp 92.85% <0%> (-3.58%) ⬇️
src/Magnum/BulletIntegration/MotionState.cpp 83.33% <0%> (-2.39%) ⬇️
src/Magnum/BulletIntegration/Integration.h 100% <0%> (ø) ⬆️
src/Magnum/ImGuiIntegration/Context.h 100% <0%> (ø) ⬆️
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
Copy link
Contributor Author

Squareys commented Jan 5, 2020

@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.

Signed-off-by: Squareys <squareys@googlemail.com>
Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge right when the CIs finish.

@mosra mosra merged commit 681d349 into mosra:master Jan 5, 2020
GUI automation moved this from TODO to Done Jan 5, 2020
@Squareys
Copy link
Contributor Author

Squareys commented Jan 5, 2020

Awesome, thanks! 🙏

@rune-scape
Copy link
Contributor

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

@mosra
Copy link
Owner

mosra commented Jan 5, 2020

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

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

4 participants