Viewports by nocheacosador · Pull Request #213 · SFML/imgui-sfml

nocheacosador · 2022-06-15T22:45:54Z

Hi!
I've just implemented ImGui multi-viewport feature for SFML. I haven't tested it with multiple ImGui contexts but with one context it seems to work. I am planning on developing it further, especially if you would consider merging it with the master branch or creating a new viewports branch in your repository. Please, don't hesitate to give me any feedback, I'm eager to contribute and learn :)
Hope to hear from you soon.

nocheacosador · 2022-06-15T22:53:57Z

Ah. Also, My implementation will only work on Windows because ImGui requires monitor info and as far as I know SFML doesn't provide any API for that, so I used Windows API for that, in the future I would be able to implement it for Ubuntu. As for MacOS it would be difficult cause I don't have Mac machine accessible to me to test the implementation.

celonymire · 2022-06-16T01:12:43Z

Thanks for your work on this! My project uses imgui-sfml and we've tried to use imgui's docking feature with no success. Will this PR allow us to use docking?

nocheacosador · 2022-06-16T01:42:09Z

Thanks for your work on this! My project uses imgui-sfml and we've tried to use imgui's docking feature with no success. Will this PR allow us to use docking?

Well, as far as I know docking should work straight out of the box, even without this PR. You just need to use docking branch of ImGui and enable docking with setting ImGuiConfigFlags_EnableDocking flag in io.ConfigFlags.

…nch.

eliasdaler · 2022-06-17T14:21:34Z

Hello.
Thanks for the PR. It's pretty complex and I can't promise that I'll be able to review it in depth soon (maybe in 2-3 weeks? Lots of stuff going on in real life, sorry...)

Is this one based on SDL backend implementation? I think that SFML and SDL implementations should be pretty close.

eliasdaler · 2022-06-17T14:22:01Z

imgui-SFML.cpp


 struct WindowContext {
-    const sf::Window* window;
+    sf::Window* const window;


This change is not needed, I like "const T*" more.

eliasdaler · 2022-06-17T14:24:01Z

imgui-SFML.cpp

 #endif

+#ifdef VIEWPORTS_ENABLE
+    const bool imContextOwner; // Context owner/main viewport


const bool should be just bool - more of a taste-thing, don't like "const" in this context.
Also the variable should be named isImContextOwner

eliasdaler · 2022-06-17T14:28:06Z

imgui-SFML.cpp

 #endif
 #endif

+#ifdef VIEWPORTS_ENABLE


I don't think that this #ifdef is needed. It's pretty confusing.
It's OK to have extra variables/constructors - this context struct is not public anyway. Usually there should be strong reason to exclude something from compilation (e.g. platform-specific things which would cause compilation to break)

Ah, I see, the WindowContext constructor might become ambiguous because of default arguments... You shouldn't make them default them to avoid ambiguity then, in my opinion.

eliasdaler · 2022-06-17T14:28:46Z

imgui-SFML.cpp


+
+// For multi-viewport support enable/disable
+#define VIEWPORTS_ENABLE


Should be named "IMGUI_SFML_VIEWPORTS_ENABLE"

Ideally you should add an option in CMake to easily turn this ON/OFF via CMake

eliasdaler · 2022-06-17T14:41:23Z

imgui-SFML.cpp

+        SFML_UpdateMonitors();
+
+        for (auto& viewport : ImGui::GetPlatformIO().Viewports) {
+            WindowContext* wc = (WindowContext*)viewport->PlatformUserData;


I don't like this copy-paste at all...
Why is it needed? Won't all events be handled by ImGui::SFML::ProcessEvent anyway?
If you need it here because of SFML_UpdateMonitors(), then maybe we need to add something like ImGui::SFML::PreUpdate and require users to call it before all ProcessEvent calls if they want to use viewports.

ImGui::SFML::ProcessEvent handles only main window events. Events for viewports' windows are not handled by it. Maybe, to avoid copy-pasting, ImGui::SFML::ProcessEvent could call internal function, for example ProcessEvent(const Event&, WindowContext*), with s_currWindowCtx, and in this loop it would be called with corresponding viewport's window context?

eliasdaler · 2022-06-17T14:43:54Z

imgui-SFML.cpp

+
+void SFML_CreateWindow(ImGuiViewport* viewport) {
+#if SFML_VERSION_MAJOR >= 3
+    sf::RenderWindow* window = new sf::RenderWindow(sf::VideoMode({(unsigned)viewport->Size.x,


Should be unsigned int, not just unsigned

eliasdaler · 2022-06-17T14:44:53Z

imgui-SFML.cpp

+}
+
+void SFML_DestroyWindow(ImGuiViewport* viewport) {
+    if (WindowContext* wc = (WindowContext*)viewport->PlatformUserData) {


Is wc ever nullptr here?

eliasdaler · 2022-06-17T14:47:37Z

imgui-SFML.cpp

+    wc->window->setPosition(pos);
+}
+
+ImVec2 SFML_GetWindowPos(ImGuiViewport* viewport) {


Why doesn't sf::Window::getPosition not work here for all platforms?

On Windows SFML sf::Window::getPosition() returns top left corner position in screen coords including title bar. ImGui requires position of client area (area where stuff is drawn), therefore if value returned by sf::Window::getPosition() is used, main viewport becomes offsetted in reference to io.MousePos by title bar height.

eliasdaler · 2022-06-17T14:47:46Z

imgui-SFML.cpp

+    MapWindowPoints(hwnd, NULL, (LPPOINT)&clientAreaRect, 2);
+    return { (float)clientAreaRect.left, (float)clientAreaRect.top };
+#else
+    if (wc->imContextOwner) return wc->window->getPosition() + sf::Vector2i(0, 24);


What is this (0, 24) offset?

This was an early attempt to cancel out that title bar offset in the main viewport. I will delete this.

eliasdaler · 2022-06-17T14:48:50Z

imgui-SFML.cpp

+
+void SFML_RenderWindow(ImGuiViewport* viewport, void*) {
+    WindowContext* wc = (WindowContext*)viewport->PlatformUserData;
+    if (!wc->imContextOwner) {


Easier to read:

if (wc->imContextOwner) { return; } ... // rest of the code

eliasdaler · 2022-06-17T14:52:11Z

@oprypin, @vittorioromeo
Would appreciate if you take a look at this if viewports/docking are something you're interested in. :)

Also, it seems to me like it would be great to separate imgui-sfml into several files now. Any thoughts?
Looks like imgui-SFML.cpp is growing way too big to handle easily...

oprypin · 2022-06-17T15:02:42Z

My implementation will only work on Windows

That makes me instantly not interested in this 😕
So I'm out, sorry

Renamed imContextOwner to isImContextOwner

Changed WindowContext contructors. Tidied-up SFML_RenderWindow

…anch

Formatting changes.

celonymire · 2022-06-19T13:33:19Z

@oprypin looks like code for Linux has been added does that interest you?

MirrasHue · 2022-08-11T02:48:37Z

Amazing, man!

I was even wondering about using the official backend (GLFW probably) to have this feature, but I prefer this one much better, so that I can stick with SFML as well.

ctag-fh-kiel · 2024-04-18T18:55:58Z

Is this pull request still active, am interested if it works with mac and could test it...

eliasdaler · 2024-04-18T23:04:48Z

Vitorrio, please don’t add me to reviews anymore.

I haven’t used SFML for many years and don’t have capacity to maintain/review ImGui-SFML stuff anymore.

nocheacosador added 3 commits June 16, 2022 01:29

Added multi-viewport feature

c33bcf7

Added code for multi-viewports

f9c5686

Ignored Visual Studio folder

669d0d9

nocheacosador added 5 commits June 16, 2022 14:57

Commented viewports code, so that example works with ImGui master bra…

ff095de

…nch.

Implemented mouse hovered viewport.

33bde7f

Included VideoMode.hpp header

603a94c

Added preprocessor statement to compile with SFML3

a274dbe

Added typecast.

1b9effb

eliasdaler requested changes Jun 17, 2022

View reviewed changes

nocheacosador added 10 commits June 17, 2022 21:54

Changed cast, from unsigned to unsigned int

7098ec1

Changed VIEWPORTS_ENABLE to IMGUI_SFML_VIEWPORTS_ENABLE

bdbedb7

Deleted const modifiers next to bool in WindowContext struct.

64a4233

Renamed imContextOwner to isImContextOwner

Removed unnecessary #ifdef in WindowContext struct.

978c1d0

Changed WindowContext contructors. Tidied-up SFML_RenderWindow

Added CMake option IMGUI_SFML_ENABLE_VIEWPORTS

a946dc0

Deleted IMGUI_SFML_VIEWPORTS_ENABLE macro definition

71b208a

Wrapped code in #ifdef to maintain compatibility with ImGui master br…

c0137fc

…anch

Deleted unnecessary code

3e17acc

Added neccessary code for Linux.

87679d6

Changed how window position is passed to ImGui.

35771ad

Formatting changes.

vittorioromeo mentioned this pull request Jun 21, 2022

Add Support for Retrieving Monitor/Screen Information SFML/SFML#2136

Open

eliasdaler mentioned this pull request Dec 9, 2022

viewport support #72

Open

vittorioromeo requested a review from eliasdaler March 6, 2024 18:48

eliasdaler requested review from a team and removed request for eliasdaler April 18, 2024 23:03

SFML deleted a comment from anthony-sv May 20, 2024



		// For multi-viewport support enable/disable
		#define VIEWPORTS_ENABLE

Conversation

nocheacosador commented Jun 15, 2022

Uh oh!

nocheacosador commented Jun 15, 2022

Uh oh!

celonymire commented Jun 16, 2022

Uh oh!

nocheacosador commented Jun 16, 2022

Uh oh!

eliasdaler commented Jun 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eliasdaler commented Jun 17, 2022

Uh oh!

oprypin commented Jun 17, 2022

Uh oh!

celonymire commented Jun 19, 2022

Uh oh!

MirrasHue commented Aug 11, 2022

Uh oh!

ctag-fh-kiel commented Apr 18, 2024

Uh oh!

eliasdaler commented Apr 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants