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

Fixed ImGui_ImplOSX_GetWindowSize and ConvertNSRect #6009

Closed
wants to merge 2 commits into from

Conversation

sivu
Copy link

@sivu sivu commented Dec 20, 2022

small fixes that i think are just copy-paste errors.

@ocornut
Copy link
Owner

ocornut commented Jan 2, 2023

Hello Mikko,

Thanks for the PR.
I have now merged the first commit as 16aaf60.

As for the second commit (9ffb69b): It is possible for mousePoint to not be in the rectangle of any screen? If e.g. mouse behavior are configured to extend outside of bounds while dragging (not sure how but assuming it is possible)

I admit am not sure I understand the intent/logic, the whole thing with flipping Y coordinates being a little confusing in this backend. In Dear ImGui view we have a single "large" coordinate space, where typically the primary monitor starts at (0,0) on Windows, and negative values are in theory (*) OK, e.g. screen left-of or above primary monitor on Windows.

Say you have two vertically stacked screens on Windows, top screen goes e.g from 0 to 1080 on Y axis, bottom screen goes from 1080 to 2160. I imagine (but I have not verified) that on Mac the top screen would go from 2160 to 1080, and bottom screen from 1080 to 0. Therefore perhaps the height we should up to do this flipping is the MAX value of all monitor. I am right? (I am not so familiar with OSX even less-so with multi-monitor OSX).

(*) in practice there's a small issues with negative coordinates but it is unrelated to the main matter here. At some point we will offset virtual space to stay in positive coordinates.

Happy new year!

@sivu
Copy link
Author

sivu commented Jan 3, 2023

As for the second commit (9ffb69b): It is possible for mousePoint to not be in the rectangle of any screen? If e.g. mouse behavior are configured to extend outside of bounds while dragging (not sure how but assuming it is possible)

To my knowledge it is not possible for the mouse coordinates be outside of any screen, they are always clamped to screen boundaries.

I admit am not sure I understand the intent/logic, the whole thing with flipping Y coordinates being a little confusing in this backend. In Dear ImGui view we have a single "large" coordinate space, where typically the primary monitor starts at (0,0) on Windows, and negative values are in theory (*) OK, e.g. screen left-of or above primary monitor on Windows.

The coordinate system is very annoying in OSX, I think everybody agrees on that :)
On OSX the main screen (internal display on laptop for example) is at 0,0 and all other displays are relative to that. I think this is how it also is on Windows. And even if you have two screens that have exact same resolution side-by-side, you can position them so that other is higher than the other. I have a setup where my external display is on the left side of my laptop, so the external display is in negative coordinates.
Screenshot 2023-01-03 at 15 50 17

And you are right, my fix only worked for me in my use-case. We need to go through all monitors and find the screen that is most top-left and use that.

@ocornut
Copy link
Owner

ocornut commented Jan 3, 2023

Thank you for confirming.

And you are right, my fix only worked for me in my use-case. We need to go through all monitors and find the screen that is most top-left and use that.

In addition, it would be worth confirming in Metrics/Debugger->Viewports that the monitors are correctly positioned.

static void ImGui_ImplOSX_UpdateMonitors()
{
    ImGuiPlatformIO& platform_io = ImGui::GetPlatformIO();
    platform_io.Monitors.resize(0);

    for (NSScreen* screen in NSScreen.screens)
    {
        NSRect frame = screen.frame;
        NSRect visibleFrame = screen.visibleFrame;

        ImGuiPlatformMonitor imgui_monitor;
        imgui_monitor.MainPos = ImVec2(frame.origin.x, frame.origin.y);
        imgui_monitor.MainSize = ImVec2(frame.size.width, frame.size.height);
        imgui_monitor.WorkPos = ImVec2(visibleFrame.origin.x, visibleFrame.origin.y);
        imgui_monitor.WorkSize = ImVec2(visibleFrame.size.width, visibleFrame.size.height);
        imgui_monitor.DpiScale = screen.backingScaleFactor;

        platform_io.Monitors.push_back(imgui_monitor);
    }
}

Multi-viewports for this backend was added by 6868d11 + d666a1d (#4821, #2778) but I'm unsure it was testing with multiple-monitors.

According to https://developer.apple.com/documentation/corefoundation/cgrect "In the default Core Graphics coordinate space, the origin is located in the lower-left corner of the rectangle and the rectangle extends towards the upper-right corner. If the context has a flipped-coordinate space—often the case on iOS—the origin is in the upper-left corner and the rectangle extends towards the lower-right corner."
May be better to use NSMinX(), NSMinY(). NSWidth(), NSHeight().

@ocornut
Copy link
Owner

ocornut commented Jan 9, 2024

I am currently on a Mac and trying to catch up with a bunch of related issues with the OSX native backend. (e.g. #6432, #7028, #7101), at the sluggish pace of a Windows user on a Mac.

It seems that everyone has a different workaround for some isolated problem but at core maybe out of sanity we should perhaps mimic SDL and GLFW in transforming coordinates so they match between all backends, starting from monitor coordinates. At the moment with the OSX backend I see different monitor coordinates as the SDL and GLFW ones.
I don't think it is reasonable to apply isolated fixes from others # because we standardize that a little better.

@sivu
Copy link
Author

sivu commented Jan 9, 2024

Agreed, I am closing this

@sivu sivu closed this Jan 9, 2024
@ocornut ocornut reopened this Jan 9, 2024
@ocornut
Copy link
Owner

ocornut commented Jan 9, 2024

Keeping open as all OSX related references are useful until this is solved.

ocornut added a commit that referenced this pull request Jan 9, 2024
@ocornut ocornut changed the title fix ImGui_ImplOSX_GetWindowSize and ConvertNSRect Fixed ImGui_ImplOSX_GetWindowSize and ConvertNSRect Jan 9, 2024
@ocornut
Copy link
Owner

ocornut commented Jan 9, 2024

I think things are now generally fixed with a683033 but it's a little difficult to reason about this, since the patch fixes others things leading to this AFAIK not being needed. At least I tested OSX backend with multi-viewports and multi-monitors in various positions and it worked (checked monitors positions, mouse position relative to what's displayed, etc.)

(few remaining issue see #7028 (comment) but generally usable)

@ocornut ocornut closed this Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants