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: make ImFontAtlas texture accessible #61

Closed
wants to merge 2 commits into from

Conversation

rune-scape
Copy link
Contributor

Fixes #60

@codecov-io
Copy link

codecov-io commented Dec 31, 2019

Codecov Report

Merging #61 into master will decrease coverage by 0.57%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
- Coverage   74.55%   73.97%   -0.58%     
==========================================
  Files          21       21              
  Lines         900      903       +3     
==========================================
- Hits          671      668       -3     
- Misses        229      235       +6
Impacted Files Coverage Δ
src/Magnum/ImGuiIntegration/Context.h 100% <ø> (ø) ⬆️
src/Magnum/ImGuiIntegration/Context.cpp 82.28% <66.66%> (-0.86%) ⬇️
src/Magnum/DartIntegration/ConvertShapeNode.cpp 93.92% <0%> (-2.21%) ⬇️

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 43e86c2...d66065c. Read the comment docs.

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.

Thank you (and extra thanks for writing the docs as well 👍).

One more thing -- could you do simplify the ternary condition we talked about? You mentioned it worked well in your last comment in #60. (To be extra sure, if you could test that also an ImGuiIntegration::image() widget still work as expected after that change, that'd be great.)

*
* @return Reference to the @cpp GL::Texture2D @ce in @cpp ImFontAtlas @ce
*/
GL::Texture2D& getAtlasTexture();
Copy link
Owner

Choose a reason for hiding this comment

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

Could this be just atlasTexture()? Magnum doesn't use get prefixes anywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, my bad

@mosra mosra added this to the 2019.1c milestone Jan 2, 2020
@mosra mosra added this to TODO in GUI via automation Jan 2, 2020
@mosra
Copy link
Owner

mosra commented Jan 3, 2020

Merged as b3c65b9, thank you! :)

@rune-scape
Copy link
Contributor Author

btw i have been testing with the ternary statement simplified, and everything works

@Squareys
Copy link
Contributor

Squareys commented Jan 5, 2020

There is a tiny bug in here:

When the Context is moved, the pointer to _texture is invalidated so that the ImGui font TexID points to invalid memory. The TexID needs to be updated in Context(Context&&).

I will follow up with a tiny PR.

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.

ImFontAtlas::TexID remains unset after a call to ImGuiIntegration::Context::relayout
4 participants