ImGui: Add support for ImGuiBackendFlags_HasSetMousePos and cleaner drawFrame() code

[Auburn]

io.WantSetMousePos will never be set unless requested by the user in the config like so:

ImGui::GetIO().ConfigFlags |= ImGuiConfigFlags_NavEnableSetMousePos;
ImGui::GetIO().ConfigFlags |= ImGuiConfigFlags_NavEnableKeyboard;

Minor tidy in DrawFrame(), the values in the ImDrawData are set based on the ImGui::GetIO() values so they will be the same

[Auburn]

I added the mouse pos update into Context::updateApplicationCursor because it was convenient and had access to the application. But it could go in it's own function.

[Auburn]

Looks like there is no way to warpCursor outside of SDL/GLFW applications, what is the best way to detect these are active?

[mosra]

There's a test that mocks up the Application class in order to be able to verify the behavior in an automated headless setting, so it'd be about adding a warpCursor() implementation around here:

magnum-integration/src/Magnum/ImGuiIntegration/Test/ContextGLTest.cpp

Line 117 in 1a66b05

void setCursor(Cursor cursor) { currentCursor = cursor; }

and then ideally also adding a new test case verifying that this API gets correctly called when it should, somewhere near to the other mouse-related tests. Hopefully you can figure that out from the existing test code, feel free to ask if not.

The nastier problem is the fallback for apps without warpCursor() as you mentioned. I think it could be implemented via some SFINAE trick, unfortunately there isn't anything remotely similar to get an inspiration from, only the cursor-related stuff. But I think something like this could work:

namespace Implementation {

template<class Application> void callWarpCursor(Application&, ...) {}
template<class Application, class = decltype(&Application::warpCursor)> void callWarpCursor(Application& application, const Vector2i& position) {
    application.warpCursor(position);
}

}

and then doing Implementation::callWarpCursor(application, ...) instead of application.warpCursor(). You'll also need some similar trick for setting the ImGuiBackendFlags_HasSetMousePos flag.

No guarantee if this works on all compilers tho :D

because it was convenient

Fine with me :)

io.WantSetMousePos will never be set unless requested by the user

Can you add this into the docs somewhere? Not sure what's the best place tho, maybe somewhere near the event handling stuff in Context.h.

[mosra]

Hm, I can't tell if this is a good or a bad change as my knowledge of ImGui is rather bad 😅 How does drawData->FramebufferScale and io.DisplayFramebufferScale relate?

Cc: @pezcode, you're responsible for most of the code here I think, and I bet you know better than me :)

[Auburn]

From the ImGui comments:

ImVec2          DisplayPos;             // Top-left position of the viewport to render (== top-left of the orthogonal projection matrix to use) (== GetMainViewport()->Pos for the main viewport, == (0.0) in most single-viewport applications)
ImVec2          DisplaySize;            // Size of the viewport to render (== GetMainViewport()->Size for the main viewport, == io.DisplaySize in most single-viewport applications)
ImVec2          FramebufferScale;       // Amount of pixels for each unit of DisplaySize. Based on io.DisplayFramebufferScale. Generally (1,1) on normal display, (2,2) on OSX with Retina display.

[Auburn]

It's mainly to aid with multi-window rendering 😉

[mosra]

I see, that makes sense then :)

[Auburn]

You'll also need some similar trick for setting the ImGuiBackendFlags_HasSetMousePos flag.

The problem with this is there is no reference to the Application class in that function

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.09 🎉

Comparison is base (1a66b05) 76.74% compared to head (36f0845) 76.83%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
+ Coverage   76.74%   76.83%   +0.09%     
==========================================
  Files          21       21              
  Lines         976      980       +4     
==========================================
+ Hits          749      753       +4     
  Misses        227      227              

Impacted Files	Coverage Δ
src/Magnum/ImGuiIntegration/Context.h	100.00% <ø> (ø)
src/Magnum/ImGuiIntegration/Context.cpp	95.48% <100.00%> (-0.03%)	⬇️
src/Magnum/ImGuiIntegration/Context.hpp	86.41% <100.00%> (+0.89%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mosra]

there is no reference to the Application class in that function

I think this could make it work -- add a new templated Context constructor overloads defined in Context.hpp that take a reference to the Application, delegate to the non-templated ones, and then set up additional flags based on what the application type supports.

// Context.h
template<class Application> explicit Context(const Vector2& size, Application& application);

template<class Application> explicit Context(ImGuiContext& context, const Vector2& size, Application& application);

// Context.hpp

// implicitly fetches framebuffer and window size from the application instance
template<class Application> Context::Context(const Vector2& size, Application& application): Context{size, application.windowSize(), application.framebufferSIze()} {
    if(application has warp cursor)
       ImGui::GetIO().BackendFlags |= ImGuiBackendFlags_HasSetMousePos;
}

template<class Application> Context::Context(ImGuiContext& context, const Vector2& size, Application& application): Context{context size, application.windowSize(), application.framebufferSIze()} {
    // same here, or maybe have some shared private helper for this instead?
}

The docs should then say something like "using this template constructor is preferrable as it enables additional features based on given Application capabilities". This way it also doesn't introduce any breakage or behavior change for existing users which is nice -- when using the original constructor, cursor warp simply won't be supported, and to make use of it, the users have to adapt their code.

Not sure yet if the original constructor should be deprecated, probably not -- as I said before, I want to eventually split this code into an event handling and rendering part so they can be better mixed with 3rd party application toolkits or 3rd party renderers, so I'll do the deprecation as part of that instead.

[mosra]

Apart from that one trailing whitespace this looks good, now just the docs :)