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

Move Imgui integration #33

Merged
merged 6 commits into from Jan 8, 2019
Merged

Move Imgui integration #33

merged 6 commits into from Jan 8, 2019

Conversation

Squareys
Copy link
Contributor

@Squareys Squareys commented Nov 12, 2018

Hi all!

This is the WIP PR for my efforts to move various magnum-imgui projects (by @lecopivo https://github.com/lecopivo/magnum-imgui and @denesik and forks by @nnarain, @williamjcm and @ShaddyDC) into the official repository.

Cheers, Jonathan

TODOs:

  • Add code from other repos
  • CMake setup
  • Add to find module
  • Migrate example => ImGuiIntegration example magnum-examples#51
    • Fix duplicate symbols issue with current find method
  • Add a test
  • Build on CI
  • Adapt Code Style
  • Add documentation
  • Re-check mentions in the license headers

@mosra mosra added this to TODO in GUI via automation Nov 12, 2018
@codecov-io
Copy link

codecov-io commented Nov 12, 2018

Codecov Report

Merging #33 into master will increase coverage by 13.37%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #33       +/-   ##
===========================================
+ Coverage   58.34%   71.72%   +13.37%     
===========================================
  Files          16       11        -5     
  Lines         557      244      -313     
===========================================
- Hits          325      175      -150     
+ Misses        232       69      -163
Impacted Files Coverage Δ
src/Magnum/ImGuiIntegration/Conversion.h 100% <100%> (ø)
src/Magnum/BulletIntegration/MotionState.cpp 78.94% <0%> (-6.77%) ⬇️
src/Magnum/BulletIntegration/DebugDraw.cpp 12.82% <0%> (-3.09%) ⬇️
src/Magnum/GlmIntegration/GtcIntegration.h 100% <0%> (ø) ⬆️
src/Magnum/DartIntegration/ConvertShapeNode.cpp
src/Magnum/DartIntegration/Object.h
src/Magnum/DartIntegration/World.h
src/Magnum/DartIntegration/World.cpp
src/Magnum/DartIntegration/Object.cpp
src/Magnum/DartIntegration/ConvertShapeNode.h
... 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 4746753...8f0b416. Read the comment docs.

@mosra mosra added this to the 2018.1d milestone Nov 12, 2018
CMakeLists.txt Show resolved Hide resolved
@mosra mosra moved this from TODO to In Progress in GUI Nov 13, 2018
@mosra
Copy link
Owner

mosra commented Nov 18, 2018

FYI, just found this: https://gitlab.com/caleb-vincent/MagnumImGui so maybe that has also some changes worth integrating (if you would open an issue there notifying the author about this merge, that would be great 👍)

@Squareys
Copy link
Contributor Author

Squareys commented Nov 19, 2018

@mosra Issues are not open on that project. Found a change, though, that I will integrate 👍

* Edit: A similiar change was already present in the other repo.

@Squareys
Copy link
Contributor Author

Squareys commented Nov 27, 2018

@mosra The code would be ready for some initial rough review pass. The documentation is not done yet, though.

(There is currently a pending issue with the example still, though, so really, just rough review... or wait another two days)

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.

This looks pretty much finished to me already :) Great work, looking forward for the final thing.

modules/FindImGui.cmake Outdated Show resolved Hide resolved
@@ -190,6 +191,14 @@ foreach(_component ${MagnumIntegration_FIND_COMPONENTS})

set(_MAGNUMINTEGRATION_${_COMPONENT}_INCLUDE_PATH_NAMES MotionState.h)

# ImGui integration library
elseif(_component STREQUAL ImGui)
find_package(ImGui REQUIRED)
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure about the REQUIRED here -- if find_package(MagnumIntegration OPTIONAL) would be used, this will break. (But OTOH I also don't know what's the proper way of doing this, I'm only saying find_package(Name) everywhere.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, right, now I remember why we had a discussion about this once :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Documentation says "REQUIRED stops processing on error message", hence manually outputting an error message would make it stop if find_package(MagnumIntegration REQUIRED ImGui) whereas OPTIONAL would print the error message but continue processing...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OTOH, the error message will be output by find_package(ImGui) already... right? So it will stop processing there, I'd expect...

Copy link
Owner

Choose a reason for hiding this comment

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

Not sure, probably not. We need to revisit this later, maybe we need to add REQUIRED based on value of MagnumIntegration_FIND_REQUIRED_ImGui ... but that then needs to be done for everything.

Copy link
Owner

Choose a reason for hiding this comment

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

Can you remove the REQUIRED, then?

@@ -65,6 +65,10 @@ install:
- IF NOT EXIST %APPVEYOR_BUILD_FOLDER%\0.9.9.0.zip appveyor DownloadFile https://github.com/g-truc/glm/archive/0.9.9.0.zip
- 7z x 0.9.9.0.zip -o%APPVEYOR_BUILD_FOLDER%\deps && ren %APPVEYOR_BUILD_FOLDER%\deps\glm-0.9.9.0 glm

# ImGui
- IF NOT EXIST %APPVEYOR_BUILD_FOLDER%\imgui-1.65.zip appveyor DownloadFile https://github.com/ocornut/imgui/archive/v1.65.zip
- 7z x v1.65.zip -o%APPVEYOR_BUILD_FOLDER%\deps && ren %APPVEYOR_BUILD_FOLDER%\deps\imgui-1.65 imgui
Copy link
Owner

Choose a reason for hiding this comment

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

Thinking out loud: we depend on particular version and since imgui backwards compatibility is broken quite often (at least by looking at changes in bgfx) it should be probably mentioned in the docs what version this integration supports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case it would almost make sense to just bundle it in external 🙈 (As submodule would be best, but that's non-trivial to use as non-git user... but if well documented...)

Copy link
Owner

Choose a reason for hiding this comment

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

Well, I hope the changes are not so drastic (and I assume it will get more stable and better backward-compatible with upcoming versions). This is similar to OVR integration, no? There it was also recommending a version but it usually worked with newer ones as well.

src/Magnum/ImGuiIntegration/Integration.h Outdated Show resolved Hide resolved
src/Magnum/ImGuiIntegration/Integration.h Outdated Show resolved Hide resolved

#include <imgui.h>

using namespace Magnum;
Copy link
Owner

Choose a reason for hiding this comment

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

No need for this here ;)

Copy link
Owner

Choose a reason for hiding this comment

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

Still not fixed ;)

src/Magnum/OvrIntegration/CMakeLists.txt Outdated Show resolved Hide resolved
src/Magnum/ImGuiIntegration/ImGui.vert Show resolved Hide resolved
src/Magnum/ImGuiIntegration/Integration.cpp Outdated Show resolved Hide resolved
src/Magnum/ImGuiIntegration/Integration.cpp Outdated Show resolved Hide resolved
@Squareys Squareys changed the title [WIP] Move Imgui integration Move Imgui integration Dec 1, 2018
@Squareys Squareys force-pushed the imgui-integration branch 3 times, most recently from c13570f to 6cde770 Compare December 1, 2018 14:04
@Squareys
Copy link
Contributor Author

Squareys commented Dec 1, 2018

@mosra Ready for review!

Open questions (/for follow up):

  • vcpkg requires support for linking imgui as lib rather than compiling in the code
  • Supported imgui version is mentioned nowhere
  • REQUIRED in FindMagnumIntegration

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.

There are still 2 unresolved comments (collapsed by GitHub) from before, one for the code snippet and one for minor code formatting. Besides that:

  • there's some dllexport/dllimport issue on MinGW. Not sure why is that?
  • the Emscripten (at least WebGL 2) build should be working too (it was working in the original repos)
  • same for iOS ES3 I think
  • there's lots of compiler warnings due to our very strict warning levels, not sure how to disable these for the interface sources (todo for myself)
  • doc/namespaces.dox is missing the license info (i'm sure i mentioned this before somewhere, but can't find where)
  • the comments below :)

I need to hurry up with the CI updates to make Travis working. I also need to test this locally before merging (+ do the "note to self" things).

doc/changelog-integration.dox Show resolved Hide resolved

_projMatrixUniform = uniformLocation("projectionMatrix");

setUniform(uniformLocation("colorTexture"), TextureLayer);
Copy link
Owner

Choose a reason for hiding this comment

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

I have a feeling that the two lines above are also wrapped in some if() for the builtin shaders. I'll look into this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So after looking into this, what do you say? (I am not sure whether I understand what this is about, btw...)

Copy link
Owner

Choose a reason for hiding this comment

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

Oh! I'll look into this during the merge, I meant.

modules/FindImGui.cmake Outdated Show resolved Hide resolved
modules/FindImGui.cmake Outdated Show resolved Hide resolved
@mosra
Copy link
Owner

mosra commented Dec 10, 2018

For the CI failures, it looks like you forgot to add the file in doc/snippets ;)

@Squareys
Copy link
Contributor Author

Squareys commented Dec 10, 2018

  • there's some dllexport/dllimport issue on MinGW. Not sure why is that?
    - Fixed seven days ago in imgui (not released yet)
  • the Emscripten (at least WebGL 2) build should be working too (it was working in the original repos)
  • same for iOS ES3 I think
  • doc/namespaces.dox is missing the license info (i'm sure i mentioned this before somewhere, but can't find where)
  • the comments below

Signed-off-by: Squareys <squareys@googlemail.com>
modules/FindImGui.cmake Outdated Show resolved Hide resolved
modules/FindImGui.cmake Outdated Show resolved Hide resolved
@Squareys Squareys force-pushed the imgui-integration branch 5 times, most recently from 482696a to d64bb8c Compare December 15, 2018 20:00
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.

Last comments from me before merge. The rest (such as ES2 / WebGL 1 shader compat) I'll do post-merge.

doc/snippets/ImGuiIntegration.cpp Outdated Show resolved Hide resolved
/* [Context-usage] */

/* [Context-usage-per-frame] */
GL::Renderer::enable(GL::Renderer::Feature::Blending);
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe also enable blending only for the draw call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume you mean disabling it again after

modules/FindImGui.cmake Outdated Show resolved Hide resolved
@@ -190,6 +191,14 @@ foreach(_component ${MagnumIntegration_FIND_COMPONENTS})

set(_MAGNUMINTEGRATION_${_COMPONENT}_INCLUDE_PATH_NAMES MotionState.h)

# ImGui integration library
elseif(_component STREQUAL ImGui)
find_package(ImGui REQUIRED)
Copy link
Owner

Choose a reason for hiding this comment

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

Can you remove the REQUIRED, then?

package/ci/appveyor.yml Outdated Show resolved Hide resolved
package/ci/travis.yml Outdated Show resolved Hide resolved
src/Magnum/ImGuiIntegration/Integration.cpp Show resolved Hide resolved

#include <imgui.h>

using namespace Magnum;
Copy link
Owner

Choose a reason for hiding this comment

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

Still not fixed ;)

src/Magnum/ImGuiIntegration/Test/IntegrationGLTest.cpp Outdated Show resolved Hide resolved
src/Magnum/ImGuiIntegration/Test/IntegrationGLTest.cpp Outdated Show resolved Hide resolved
@Squareys Squareys force-pushed the imgui-integration branch 3 times, most recently from 125bf31 to d49c2e9 Compare December 16, 2018 00:48
- Add documentation
- Tests for input and basic test for rendering
- Adapt code style
- Add Conversion.h for converting imgui math types
- Compile shaders as resource
- Add find module for ImGui

Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
@mosra mosra mentioned this pull request Dec 29, 2018
55 tasks
@williamjcm
Copy link
Contributor

williamjcm commented Jan 4, 2019

Would it be possible for the official integration to have a way to load custom fonts ?

I currently have a customised version of nnarain's integration, and I had to edit it to add code to load my own fonts.

I went to this line, and added the following before it:

io.Fonts->AddFontFromFileTTF("aller.ttf",
                             16.0f,
                             nullptr,
                             ranges.Data);
io.Fonts->AddFontFromFileTTF("aller.ttf",
                             18.0f,
                             nullptr,
                             ranges.Data);
io.Fonts->AddFontFromFileTTF("aller.ttf",
                             20.0f,
                             nullptr,
                             ranges.Data);
io.Fonts->AddFontFromFileTTF("aller.ttf",
                             22.0f,
                             nullptr,
                             ranges.Data);

As you can see, I load the same font multiple times at different sizes (to fit users' needs).

I don't specify a custom font config (hence the nullptr as the third parameter), but I use a custom character range (the ranges variable) to add support for all the glyphs in the font. Therefore, having both as optional parameters in the integration could be a possibility.

Here's the ImGui documentation about fonts: https://github.com/ocornut/imgui/blob/67775448556d8dba304657719829f1ab768b3c97/imgui.cpp#L764-L838 and https://github.com/ocornut/imgui/blob/master/misc/fonts/README.txt

@Squareys
Copy link
Contributor Author

Squareys commented Jan 4, 2019

@williamjcm You can still add those lines after calling the Magnum::ImGuiIntegration::Context constructor in your user code. If you use ImGui::GetIO(), you will retrieve the same IO struct that is being used by Magnum.

Otherwise a more magnum-like solution (? @mosra ) would probably be to allow ImGui to use Magnum::Text::AbstractFont somehow, but since this is likely quite a bit of work, it will definitely not be part of this PR.

@williamjcm
Copy link
Contributor

williamjcm commented Jan 4, 2019

The code that generates the font texture data is in the integration, however.

Link to nnarain's version of the integration: https://github.com/nnarain/magnum-imgui/blob/256968783121306c65ddfb5c28906140f2ccf947/src/MagnumImGui.cpp#L76-L84

@Squareys
Copy link
Contributor Author

Squareys commented Jan 6, 2019

Oh! True, sorry, thanks for the hint!

@mosra Maybe it would be best to add a dependency on AbstractFont here then and let that generate the glyph texture instead? I wouldn't expect the textures to be compatible, though, so maybe a "generateFonts" or something as a second step after Context initialization. I wonder how we could make that "unforgetible", i.e. an optional second step if no custom font needs to be loaded 🤔

@williamjcm
Copy link
Contributor

williamjcm commented Jan 6, 2019

Maybe two constructors, with one asking for a file name among its parameter ?

And once the object is constructed, a method could allow generating even more fonts as needed, maybe with optional arguments like ImGui font config and character ranges.

@mosra
Copy link
Owner

mosra commented Jan 6, 2019

Shower thoughts: I finally can see a bit into how this whole thing works, so if I understand correctly, all you need is to be able to do some stuff between ImGui::CreateContext() and io.Fonts->GetTexDataAsRGBA32(), right?

So let's no overdo this. Could the following new constructor cover your case? (the parameter is just to distinguish the intention, otherwise not really needed)

image

@williamjcm
Copy link
Contributor

I like that idea.

It would give more control to the integration's users who want it, while still giving a "I don't care" option to those who just want to get ImGui up and running.

@mosra
Copy link
Owner

mosra commented Jan 8, 2019

I merged the PR with the following changes:

  • The headers are renamed to Context.h for the Context class and Integration.h for the math integration, to be more in-line with the naming elsewhere
  • The above proposal for custom fonts is integrated
  • The API had to be changed for DPI awareness, so instead of nextFrame() taking window / framebuffer size, a tuple of UI size, window size and framebuffer size is passed to the constructor and there's a new relayout() function to handle window size or pixel density changes. This is basically equivalent to how it's done in the Ui library, so you can just reuse the logic from there. The docs (which I'll be uploading later) explain the interactions of DPI awareness with custom fonts.
  • The singleton and get() is no more, instead you are expected to handle a singleton yourself, if needed -- when working on the example I realized that the singleton was not needed at all, since the only place where the nextFrame() / drawFrame() and event functions were called was in the main app code, which also is the owner of the Context instance. This allowed me to make it move-aware, have a NoCreate constructor and support multiple contexts, and also removing all restrictive singleton-related assertions from the internals.
  • The image() widget now has the UV range represented as Range2D instead of a pair of vectors.
  • There's also conversion between Color3 and ImColor, to allow things like ImColor(0xff3366_rgbf) (and avoid crazy implicit-conversion-caused bugs, more in the docs)
  • Mouse position is registered also for mouse move / scroll events, for some reason it was done only for mouse move event, which would cause issues on touchscreens (which have no move events reported)
  • The Context class is now marked as experimental, since I'm not really sure how to implement a Vulkan backend yet and it might involve some renaming, so better safe than sorry ;)

I did not bother with port to ES2 / WebGL 1 yet since that would delay the merge by another year, will do that later. I'm now working on the example, will upload the docs once I have the example updated and merged as well.

Feedback and bugreports very appreciated :)

@mosra mosra merged commit 8f0b416 into mosra:master Jan 8, 2019
GUI automation moved this from In Progress to Done Jan 8, 2019
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