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

Viewports #213

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Viewports #213

wants to merge 18 commits into from

Conversation

nocheacosador
Copy link

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
Copy link
Author

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.

@sam20908
Copy link

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
Copy link
Author

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.

@eliasdaler
Copy link
Contributor

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.

@@ -176,7 +185,7 @@ struct TriggerInfo {
};

struct WindowContext {
const sf::Window* window;
sf::Window* const window;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

imgui-SFML.cpp Outdated
@@ -207,15 +216,28 @@ struct WindowContext {
#endif
#endif

#ifdef VIEWPORTS_ENABLE
const bool imContextOwner; // Context owner/main viewport
Copy link
Contributor

Choose a reason for hiding this comment

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

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

imgui-SFML.cpp Outdated
@@ -207,15 +216,28 @@ struct WindowContext {
#endif
#endif

#ifdef VIEWPORTS_ENABLE
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

imgui-SFML.cpp Outdated
@@ -24,6 +25,14 @@
#include <memory>
#include <vector>


// For multi-viewport support enable/disable
#define VIEWPORTS_ENABLE
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Should be named "IMGUI_SFML_VIEWPORTS_ENABLE"
  2. Ideally you should add an option in CMake to easily turn this ON/OFF via CMake

SFML_UpdateMonitors();

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

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

imgui-SFML.cpp Outdated

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

Choose a reason for hiding this comment

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

Should be unsigned int, not just unsigned

}

void SFML_DestroyWindow(ImGuiViewport* viewport) {
if (WindowContext* wc = (WindowContext*)viewport->PlatformUserData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is wc ever nullptr here?

Copy link
Author

Choose a reason for hiding this comment

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

No.

wc->window->setPosition(pos);
}

ImVec2 SFML_GetWindowPos(ImGuiViewport* viewport) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

imgui-SFML.cpp Outdated
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this (0, 24) offset?

Copy link
Author

Choose a reason for hiding this comment

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

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

imgui-SFML.cpp Outdated

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

Choose a reason for hiding this comment

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

Easier to read:

if (wc->imContextOwner) {
   return;
}

... // rest of the code

@eliasdaler
Copy link
Contributor

@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
Copy link
Member

oprypin commented Jun 17, 2022

My implementation will only work on Windows

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

@sam20908
Copy link

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

@MirrasHue
Copy link

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
Copy link

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

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

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.

@SFML SFML deleted a comment from anthony-sv May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants