Acquire size of ImGUI canvas from game viewport

[kkawachi]

This change adjusts the size of ImGUI canvas to the game viewport.

The current implementation draws ImGUI widgets onto large 4K canvas. This causes following issues in smaller viewports:

• Modal popup windows (ImGui::BeginPopupModal()) pops up in the center of 4K canvas. In working with 2K displays, they cannot be visible without dragging or zooming out the canvas.
• Pulldown menus of ImGui run out of the bottom of the game viewport and cannot be clickable.

The pull request adjusts IO.DisplaySize of ImGUI every frame so that DisplaySize matches the game viewport in order to solve these problems.

Best,

[segross]

Hey, thank you for your change, but could that be an option?

I was considering limiting canvas size but in the end decided to use a bigger one (possibly too big) and use this (https://github.com/segross/UnrealImGui#navigating-around-imgui-canvas) to navigate around it. The reasons why canvas is not linked to the size of viewport: 1) Resizing canvas can move ImGui widgets what means that binding it to the viewport size could constantly change debug layout, for instance when pressing F11 in PIE window. 2) I tend to use lots of windows when debugging. I like to keep some of those widets out of the way and them bring them back when necessary. With canvas size the same as game viewport that woud be not possible. 3) And I know it is a bit silly to discuss unimplemented features but I wanted to allow sharing canvas with ImGuiRemote or between different widgets with potentially different size (for debugging outside of the game viewport). I just didn't find a time to clean that quick RemoteImGui integration that I have.

For modals, I think it is possible to draw them at custom positions, but I see that limiting canvas size might be a better option is some scenarios. If that was optional (especially in the editor), it would be great.

On a side note, I think ImGui IO has a visible area property (or something like that, I don't remember exactly), which seems like something that should allow fixing what you described without limiting the canvas size. Did you actually try that? I think I did it a long time ago but it was new and not 100% reliable at that time.

Best regards

[kkawachi]

Thank you for reviewing my PR. I understand the design goal of the current implementation by your description. I'll update the PR and let you know soon.

[segross]

Thanks for understanding :) As I wrote in the other thread, pleas be patient with me as right now I have limited capabilities to work with this repository (any repository actually but I only maintain this one :)

[kkawachi]

Hi, I've updated the branch for adding a command ImGui.ToggleCanvasSizeAdaptive to enable/disable the variable canvas size. The default behavior is compatible with the current implementation. Would you mind reviewing the changes when you have time?

As for the visible area property of ImGui you mentioned I couldn't spot it, so the PR is implemented based on ImGui's IO.DisplaySize.

[segross]

Hey, thanks for the update. I will try to look at this in the next two days. Sorry for keeping you waiting so long.

[segross]

Hey, thank you for your update. I made a quick review but I have found two major issues. One might be seen as a bit pedantic but since this is potentially unsafe (more in the comment to the code). The other one is, unfortunately, a bit worse as it basically means that the feature will not work as expected in multi-PIE sessions. This is connected with how worlds and widgets are updated. Normally I deal with it by cashing data in a context proxy and then copying everything when starting a new frame (in either direction).

This is a nice change but it would need addressing those two issues, especially the second one. If I'm not clear about something, let me know and I will be happy to explain more.

Additionally, do you think that it is better for bCanvasSizeAdaptive to be a property or a setting? If I recall correctly properties are for switches that are expected to dynamically change (toggling input or the demo, etc.) and settings are meant for more persistent values (input handler, shortcuts, etc.). As always, there are some blurred lines and this might fall in that area but I would see it more as a setting. If this is a property and you don't like the default value, then you have to always change it after starting the application, what might be a bit hassle. On the other hand, a setting can be changed once in the Project Settings and it will be remembered every time when you start the application.

[kkawachi]

Thank you for reviewing and comments. I updated the branch to enable this feature by a setting instead of a property, so the incomplete object is not passed in ctors any more.

As for SImGuiWidget::ComputeDesiredSize(), I noticed there is no need to zoom in/out when adaptive canvas size is enabled by a setting (because the canvas size equals to the viewport size), so I've reverted this function to original implementation.

As for multi-PIE sessions, frankly speaking I've never considered it in writing this feature, but I believe the current PR is helpful for single PIE in a small window. What do you think of it?

[segross]

Hey, the problem is more with the fact that it actually breaks the multi-PIE. However, even if it didn't break multi-PIE, I would have to be careful with a change that doesn't cover all the engine or editor use cases as from my experience, I can say that sooner or later somebody will open an issue asking to fix it.

If I get some time I can try to check it more closely as I expect that it might be actually a small modification. If I can find it, I might even take that change and amend it.

[kkawachi]

Thanks. My comment was somewhat ambiguous and may have confused you, but the latest pull request (585f693) never breaks compatibility with the original implementation because the new feature (adaptive canvas size) is enabled only when bAdaptiveCanvasSize is set true. I would appreciate it if you could review it when you have a time.

[segross]

Hey, sorry for being a bit unresponsive but I'm just preoccupied with other stuff. I was expecting you can say that and to be fair it is true to a point. Combining that with a fact that I didn't put guidance on how to contribute, I can see your point... although I don't fully agree :) If I find some time, I will try merging it in the next few days. Given that I was planning a similar feature I can use this as a trigger and a starting point and add what I want.

And now, the reason why I care, is that I want all features to support all the main working scenarios in Unreal as much as possible. Of course, this is not always possible or easy and sometimes there might be a compelling reason not to but in general, I see partially supported features as a WIP. Allowing that by default would mean that when seeing a bunch of features it would be hard to say which one can be safely used in a particular workflow. And from my experience, it would also mean more discussions in the issue threads.

Luckily, in this case, it will be easy to change as the main problem is the place where data are read. Doing that in the widget is easier because it has access to the correct viewport and to proxy where it can copy data. Additionally, it can be nicely aligned with a feature that I'm planning to do sometime later (quite later knowing life). I've already tried that on top of your changes and now need to clean it and merge.

[segross]

Hey @kkawachi, I have finally merged that pull request. Thank you for your changes and patience. Sorry that it took so long. That feature is part of the version 1.19. I've added multi-PIE support, and it works fine. I also want to add a possibility to define own fixed canvas size, so 1.19 is still marked as WIP. This is something that I was thinking about and you just triggered it :)

Because of the different modes, I might change that bool property to an enum. If I change it, sorry in advance as you might need to re-enable your adaptive canvas size. Current property is fine but with more settings in place, enum might be more readable. It will look like the Game Viewport Settings in Play Advanced Settings.

[kkawachi]

Thank you for your update and merger! I've confirmed the latest master works beautifully.

[segross]

Hey @kkawachi. I have finally found a time to add those additional canvas options and integrate that with a basic DPI scale that I have added. But the setup changed slightly and the next time that you sync you will need to update your settings. To get the adaptive canvas size, just set the Canvas Size property to Viewport. I'm sorry for that inconvenience, but with other options in place, combining them together seems the most convenient from the UX perspective.

[kkawachi]

@segross , thanks for the heads-up!