Fix #111295: Add Missing Win32 Platform-Specific Window functions #111359

Merged
Harley Acheson merged 6 commits from Harley/blender:Win32WinFunctions into main 2023-08-25 22:10:02 +02:00
Member

Add platform-specific (Windows) versions of getOSWindow() and
getWindowUnderCursor().


This does not add any new ghost functions, just Windows-specific implementations of existing ones.

The base default behavior of getOSWindow always returns nullptr. This PR makes it return the Windows OS Handle as expected. This is needed for getWindowAssociatedWithOSWindow to work correctly because it uses this as a test while iterating through ghost windows. So it currently always returns nullptr.

The base default behavior of getWindowUnderCursor does a forward search through the ghost windows checking bounds. This does not consider z-depth. And because the search is forward it will always return the main window if a child is overlapping in front. This specific implementation gets the cursor location from the OS and then the Window handle from that using WindowFromPoint. Converting from window handle to ghost window requires getWindowAssociatedWithOSWindow, which needs a working getOSWindow.

This PR does not use the passed cursor location because that will often be wrong. The best example is when we have a blender window on one monitor but the mouse cursor is on a different monitor that uses a different scale. In this case the cursor location will not be a nice offset from our blender window. Best to just ask the OS where the cursor actually is.

For testing and how this relates to the referenced bug report. Opening the "Preferences" window and having it above the main window, using the color picker in the Themes area will select through to the main window. This is because the base getWindowUnderCursor behavior returns the first window with bounds that match the cursor position.

Add platform-specific (Windows) versions of getOSWindow() and getWindowUnderCursor(). --- This does not add any new ghost functions, just Windows-specific implementations of existing ones. The base default behavior of `getOSWindow` always returns nullptr. This PR makes it return the Windows OS Handle as expected. This is needed for `getWindowAssociatedWithOSWindow` to work correctly because it uses this as a test while iterating through ghost windows. So it currently always returns nullptr. The base default behavior of `getWindowUnderCursor` does a forward search through the ghost windows checking bounds. This does not consider z-depth. And because the search is forward it will always return the main window if a child is overlapping in front. This specific implementation gets the cursor location from the OS and then the Window handle from that using `WindowFromPoint`. Converting from window handle to ghost window requires `getWindowAssociatedWithOSWindow`, which needs a working `getOSWindow`. This PR does not use the passed cursor location because that will often be wrong. The best example is when we have a blender window on one monitor but the mouse cursor is on a different monitor that uses a different scale. In this case the cursor location will not be a nice offset from our blender window. Best to just ask the OS where the cursor actually is. For testing and how this relates to the referenced bug report. Opening the "Preferences" window and having it above the main window, using the color picker in the Themes area will select through to the main window. This is because the base getWindowUnderCursor behavior returns the first window with bounds that match the cursor position.
Harley Acheson added 1 commit 2023-08-21 20:25:01 +02:00
6bf593b3ad Fix #111295: Add Missing Win32 Platform-Specific Window functions
Add platform-specific (Windows) versions of getOSWindow() and
getWindowUnderCursor().

---

The base default behavior of `getOSWindow` always returns nullptr. This PR makes it return the Windows OS Handle as expected. This is needed for `getWindowAssociatedWithOSWindow` to work correctly, otherwise that always returns nullptr.

The base default behavior of `getWindowUnderCursor` does a forward search through the ghost windows checking bounds. This does not consider z-depth. And because the search is forward it will always return the main window if a child is overlapping in front. This specific implementation gets the cursor location from the OS and then the Window from that.

This PR does not use the passed cursor location because that will often be wrong. The best example is when we have a blender window on one monitor but the mouse cursor is on a different monitor that uses a different scale. In this case the cursor location will not be a nice offset from our blender window. Best to just ask the OS where the cursor actually is.

For testing and how this relates to the referenced bug report. Opening the "Preferences" window and having it above the main window, using the color picker in the Themes area will select through to the main window. This is because the base getWindowUnderCursor returns the first window with bounds that match the cursor position.
Harley Acheson added this to the Platforms, Builds Tests & Devices project 2023-08-21 20:25:11 +02:00
Harley Acheson requested review from Ray molenkamp 2023-08-21 20:25:20 +02:00
Ray molenkamp requested changes 2023-08-21 21:44:58 +02:00
Ray molenkamp left a comment
Member

I gotta say, i'm not loving the x,y coordinates being ignored, then again i'm also not loving the initial design, where a function called getWindowUnderCursor needs x,y coordinates fed to it, feels like something it should be able to figure out on its own. Or perhaps the function should have been called getWindowForScreenCoordinates in the first place if the location of the cursor has no influence on the results what so ever.

from talking with you on chat, we seem a bit stuck with this mess, without time to fix it, as some UI theming work needs this to function, to move forward i see 3 options :

  1. this function needs a better comment in both GHOST_ISystem.hh and GHOST_C-api.h outlining that the x and y parameters are hints at best, and could be ignored on some backends.

  2. Change the signature not to take x,y coordinates. I'm unsure why they are needed, @mano-wii may have some historic knowledge here as he implemented this in the past.

  3. Make it actually work with the x,y coordinates, I understand per monitor DPI scaling makes this difficult/impossible

1 is the bare minimum we need to land this since it's a blocker, 2 or 3 would be my preference though

I gotta say, i'm not loving the x,y coordinates being ignored, then again i'm also not loving the initial design, where a function called `getWindowUnderCursor` needs x,y coordinates fed to it, feels like something it should be able to figure out on its own. Or perhaps the function should have been called `getWindowForScreenCoordinates` in the first place if the location of the cursor has no influence on the results what so ever. from talking with you on chat, we seem a bit stuck with this mess, without time to fix it, as some UI theming work needs this to function, to move forward i see 3 options : 1) this function needs a better comment in both `GHOST_ISystem.hh` and `GHOST_C-api.h` outlining that the x and y parameters are hints at best, and could be ignored on some backends. 2) Change the signature not to take x,y coordinates. I'm unsure why they are needed, @mano-wii may have some historic knowledge here as he implemented this in the past. 3) Make it actually work with the x,y coordinates, I understand per monitor DPI scaling makes this difficult/impossible 1 is the bare minimum we need to land this since it's a blocker, 2 or 3 would be my preference though

To bring a little historical context.
Prior to 9bd586a01e all OS's had the same solution for finding a window under the cursor.

But the existing solution there didn't work for MacOS when the cursor was on another monitor (bringing bugs).

It was not possible to use the same generic solution for Mac. So it was necessary to create a specific solution for this OS.

Thus, the algorithm that was previously in WM_window_find_under_cursor was then moved to GHOST_IWindow *GHOST_SystemWin32::getWindowUnderCursor;

And on Mac another algorithm using OS API overwrote the existing one.

To bring a little historical context. Prior to 9bd586a01e all OS's had the same solution for finding a window under the cursor. But the existing solution there didn't work for MacOS when the cursor was on another monitor (bringing bugs). It was not possible to use the same generic solution for Mac. So it was necessary to create a specific solution for this OS. Thus, the algorithm that was previously in `WM_window_find_under_cursor` was then moved to `GHOST_IWindow *GHOST_SystemWin32::getWindowUnderCursor`; And on Mac another algorithm using OS API overwrote the existing one.
Harley Acheson added 1 commit 2023-08-21 22:43:57 +02:00
Author
Member

But the existing solution there didn't work for MacOS when the cursor was on another monitor (bringing bugs).

Not on Mac here, but to my eye I think it might also benefit from asking the OS where the mouse is, instead of using the coordinates we supply. Something like:

diff --git a/intern/ghost/intern/GHOST_SystemCocoa.mm b/intern/ghost/intern/GHOST_SystemCocoa.mm
index b2fa4de47b9..bb419cbbd62 100644
--- a/intern/ghost/intern/GHOST_SystemCocoa.mm
+++ b/intern/ghost/intern/GHOST_SystemCocoa.mm
@@ -809,7 +809,7 @@ GHOST_TSuccess GHOST_SystemCocoa::disposeContext(GHOST_IContext *context)
 
 GHOST_IWindow *GHOST_SystemCocoa::getWindowUnderCursor(int32_t x, int32_t y)
 {
-  NSPoint scr_co = NSMakePoint(x, y);
+  NSPoint scr_co = CGEventGetLocation(CGEventCreate(NULL));
 
   int windowNumberAtPoint = [NSWindow windowNumberAtPoint:scr_co belowWindowWithWindowNumber:0];
   NSWindow *nswindow = [NSApp windowWithWindowNumber:windowNumberAtPoint];

> But the existing solution there didn't work for MacOS when the cursor was on another monitor (bringing bugs). Not on Mac here, but to my eye I _think_ it might also benefit from asking the OS where the mouse is, instead of using the coordinates we supply. Something like: ```Diff diff --git a/intern/ghost/intern/GHOST_SystemCocoa.mm b/intern/ghost/intern/GHOST_SystemCocoa.mm index b2fa4de47b9..bb419cbbd62 100644 --- a/intern/ghost/intern/GHOST_SystemCocoa.mm +++ b/intern/ghost/intern/GHOST_SystemCocoa.mm @@ -809,7 +809,7 @@ GHOST_TSuccess GHOST_SystemCocoa::disposeContext(GHOST_IContext *context) GHOST_IWindow *GHOST_SystemCocoa::getWindowUnderCursor(int32_t x, int32_t y) { - NSPoint scr_co = NSMakePoint(x, y); + NSPoint scr_co = CGEventGetLocation(CGEventCreate(NULL)); int windowNumberAtPoint = [NSWindow windowNumberAtPoint:scr_co belowWindowWithWindowNumber:0]; NSWindow *nswindow = [NSApp windowWithWindowNumber:windowNumberAtPoint]; ```

I remember having tested it on monitors with different pixel resolution on the Mac. I don't really understand this limitation.

If that's really the case, it looks like the function wm_cursor_position_to_ghost(win, &x, &y); is incorrect or limited.

Since the cursor isn't used, calling wm_cursor_position_to_ghost seems to be redundant, (which doesn't look good on something that gets called so often).

Also since this function is working incorrectly, the other associated function wm_cursor_position_from_ghost(r_win, &x, &y); must also be incorrect.

So even if the window is the correct one, the relative coordinates returned by WM_window_find_under_cursor are incorrect.

So it would be ideal to fix wm_cursor_position_to_ghost and wm_cursor_position_from_ghost (or remove the need for them for all OS's).

I remember having tested it on monitors with different pixel resolution on the Mac. I don't really understand this limitation. If that's really the case, it looks like the function `wm_cursor_position_to_ghost(win, &x, &y);` is incorrect or limited. Since the cursor isn't used, calling `wm_cursor_position_to_ghost` seems to be redundant, (which doesn't look good on something that gets called so often). Also since this function is working incorrectly, the other associated function `wm_cursor_position_from_ghost(r_win, &x, &y);` must also be incorrect. So even if the window is the correct one, the relative coordinates returned by `WM_window_find_under_cursor` are incorrect. So it would be ideal to fix `wm_cursor_position_to_ghost` and `wm_cursor_position_from_ghost` (or remove the need for them for all OS's).
Member

FYI I'm not sure this patch handles that, but #111295 is also reproducible on linux, so it's probably also broken on at least X11. (My wayland is broken can't test it)

FYI I'm not sure this patch handles that, but #111295 is also reproducible on linux, so it's probably also broken on at least X11. (My wayland is broken can't test it)
Author
Member

@ChengduLittleA - FYI I'm not sure this patch handles that, but #111295 is also reproducible on linux, so it's probably also broken on at least X11. (My wayland is broken can't test it)

Assuming you mean that when Properties is on top of the main blender window the picker will select from the main window. That is probably a strong argument for having the loop reversed in GHOST_System.cc, GHOST_System::getWindowUnderCursor. That way the default behavior would test children before their parents. That would be better in almost all cases.

The loop there now is a "foreach" style (range-based) for loop, which AFAIK does not offer some easy "reverse" option. So just needs to be rewritten in regular for style.

> @ChengduLittleA - FYI I'm not sure this patch handles that, but #111295 is also reproducible on linux, so it's probably also broken on at least X11. (My wayland is broken can't test it) Assuming you mean that when Properties is on top of the main blender window the picker will select from the main window. That is probably a strong argument for having the loop reversed in GHOST_System.cc, GHOST_System::getWindowUnderCursor. That way the default behavior would test children before their parents. That would be better in almost all cases. The loop there now is a "foreach" style (range-based) for loop, which AFAIK does not offer some easy "reverse" option. So just needs to be rewritten in regular for style.
Harley Acheson added 2 commits 2023-08-25 19:55:55 +02:00
Author
Member

@blender-bot build

@blender-bot build
Harley Acheson added 2 commits 2023-08-25 21:50:52 +02:00
Ray molenkamp approved these changes 2023-08-25 22:01:48 +02:00
Harley Acheson merged commit f69c7afe57 into main 2023-08-25 22:10:02 +02:00
Harley Acheson deleted branch Win32WinFunctions 2023-08-25 22:10:06 +02:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#111359
No description provided.