Fix eyedropper outside blender on mac #115187

Merged
Harley Acheson merged 2 commits from rajveermalviya/blender:eyedrop-mac into main 2023-12-13 22:03:05 +01:00
Contributor

Updates #111303 - adding support for picking color from outside of blender window on macOS, but there are limitations (similar to X11 impl):

  • User needs to allow screen capture permission to Blender.
  • Cursor icon can't be eyedropper outside the window.
  • User needs to press Enter to trigger the picker.

This differs from GIMP where it listens to mouse events and updates the color continuously (which results in macOS assuming it's doing a screen recording and shows the screen recording indicator in the taskbar).

Here's the current behavior:

Updates #111303 - adding support for picking color from outside of blender window on macOS, but there are limitations (similar to X11 impl): - User needs to allow screen capture permission to Blender. - Cursor icon can't be eyedropper outside the window. - User needs to press <kbd>Enter</kbd> to trigger the picker. This differs from GIMP where it listens to mouse events and updates the color continuously (which results in macOS assuming it's doing a screen recording and shows the screen recording indicator in the taskbar). Here's the current behavior: <video src="/attachments/a11ccd4f-1221-4277-9494-2478f4534763" title="output_video.mp4" controls></video>
Rajesh Malviya force-pushed eyedrop-mac from 15801b1606 to 734a86d88c 2023-11-20 15:13:40 +01:00 Compare
Iliya Katushenock added this to the User Interface project 2023-11-20 15:18:15 +01:00
Rajesh Malviya changed title from WIP: Fix eyedropper outside blender on mac to Fix eyedropper outside blender on mac 2023-11-20 15:22:14 +01:00
Member

@blender-bot build macos

@blender-bot build macos
Member

@blender-bot build macos package

@blender-bot build macos package
Member

Unknown platform package. See documentation for details.

Unknown platform `package`. See [documentation](https://projects.blender.org/infrastructure/blender-bot/src/branch/main/README.md) for details.
Member

@blender-bot package macos

@blender-bot package macos
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR115187) when ready.
Member

@rajveermalviya

Nice!

Your uses of line comments with "//" in GHOST_SystemCocoa::getPixelAtCursor should be changed to block comments delimited by "/⁎" and "⁎/"

This compiles okay, and I made builds of it available for testing here:

Because our DevTalk site is currently under DDOS attack, I created a call for testing on BlenderArtists here: https://blenderartists.org/t/testing-requested-for-mac-eyedropper-builds/1494179

@rajveermalviya Nice! Your uses of line comments with "//" in GHOST_SystemCocoa::getPixelAtCursor should be changed to block comments delimited by "/⁎" and "⁎/" This compiles okay, and I made builds of it available for testing here: * [Apple Silicon](https://builder.blender.org/download/patch/blender-4.1.0-alpha+main-PR115187.734a86d88cee-darwin.arm64-release.dmg) * [Intel](https://builder.blender.org/download/patch/blender-4.1.0-alpha+main-PR115187.734a86d88cee-darwin.x86_64-release.dmg) Because our DevTalk site is currently under DDOS attack, I created a call for testing on BlenderArtists here: https://blenderartists.org/t/testing-requested-for-mac-eyedropper-builds/1494179
Rajesh Malviya force-pushed eyedrop-mac from 734a86d88c to 256e0e43c7 2023-11-23 06:54:51 +01:00 Compare
Author
Contributor

Your uses of line comments with "//" in GHOST_SystemCocoa::getPixelAtCursor should be changed to block comments delimited by "/⁎" and "⁎/"

Updated!

> Your uses of line comments with "//" in GHOST_SystemCocoa::getPixelAtCursor should be changed to block comments delimited by "/⁎" and "⁎/" Updated!
Member

@rajveermalviya - Updated!

Thanks!

There has only been one person responding to the thread on Blenderartists so far, here: https://blenderartists.org/t/testing-requested-for-mac-eyedropper-builds/1494179

They are reporting some successes and some returns of (0,0,0). That return is just the default returned if your getPixelAtCursor returns GHOST_kFailure. So my gut feeling is that will be something simple.

The first part that looks suspicious is when there is a call to CGGetDisplaysWithPoint in order to figure out which display contains the mouse location. You are passing 1 as the second argument and therefore getting only the first display that contains the point. But the docs for that imply that more than one display can contain the location and that there are times you might call that function with maxDisplays set to zero or with dspys set to NULL. You might want to pass an array to dspys of size maxDisplays and then examine the return of dspyCnt. https://developer.apple.com/documentation/coregraphics/1454385-cggetdisplayswithpoint

You could even consider using the displayid of the main display if you fail with the above.

> @rajveermalviya - Updated! Thanks! There has only been one person responding to the thread on Blenderartists so far, here: https://blenderartists.org/t/testing-requested-for-mac-eyedropper-builds/1494179 They are reporting some successes and some returns of (0,0,0). That return is just the default returned if your getPixelAtCursor returns GHOST_kFailure. So my gut feeling is that will be something simple. The first part that looks suspicious is when there is a call to CGGetDisplaysWithPoint in order to figure out which display contains the mouse location. You are passing `1` as the second argument and therefore getting only the first display that contains the point. But the docs for that imply that more than one display can contain the location and that there are times you might call that function with maxDisplays set to zero or with dspys set to NULL. You might want to pass an array to dspys of size maxDisplays and then examine the return of dspyCnt. https://developer.apple.com/documentation/coregraphics/1454385-cggetdisplayswithpoint You could even consider using the displayid of the main display if you fail with the above.
Rajesh Malviya force-pushed eyedrop-mac from 256e0e43c7 to 7442892cee 2023-11-27 20:53:38 +01:00 Compare
Author
Contributor

They are reporting some successes and some returns of (0,0,0). That return is just the default returned if your getPixelAtCursor returns GHOST_kFailure. So my gut feeling is that will be something simple.

Ah, I hadn't tested with external monitor, now did it and it's seems to be failing for me as well, I tried to fix it with current impl but CGDisplayCreateImageForRect kept returning nil when trying to capture the secondary extended display.
I tried following fixes, with no luck:

  • used CGGetDisplaysWithRect instead of CGGetDisplaysWithPoint
  • CGGetDisplaysWithRect(rect, maxDisplayCount, displays, &displayCount) and looping over the displays array with CGDisplayCreateImageForRect.

At the end I updated the impl to use CGWindowListCreateImage instead, and it works correctly for me, even with the secondary extended displays. But do note that CGWindowListCreateImage got deprecated in macOS 14 (Sonoma) but it's still available - the recommendation is to use ScreenCaptureKit API instead but SCScreenshotManager (what we need) is only available in macOS 14+ & it's callback based.

IMO we should keep using CGWindowListCreateImage and migrate to the new API later.

> They are reporting some successes and some returns of (0,0,0). That return is just the default returned if your getPixelAtCursor returns GHOST_kFailure. So my gut feeling is that will be something simple. Ah, I hadn't tested with external monitor, now did it and it's seems to be failing for me as well, I tried to fix it with current impl but `CGDisplayCreateImageForRect` kept returning `nil` when trying to capture the secondary extended display. I tried following fixes, with no luck: - used `CGGetDisplaysWithRect` instead of `CGGetDisplaysWithPoint` - `CGGetDisplaysWithRect(rect, maxDisplayCount, displays, &displayCount)` and looping over the displays array with `CGDisplayCreateImageForRect`. At the end I updated the impl to use `CGWindowListCreateImage` instead, and it works correctly for me, even with the secondary extended displays. But do note that `CGWindowListCreateImage` got deprecated in macOS 14 (Sonoma) but it's still available - the recommendation is to use ScreenCaptureKit API instead but [`SCScreenshotManager`](https://developer.apple.com/documentation/screencapturekit/scscreenshotmanager) (what we need) is only available in macOS 14+ & it's callback based. IMO we should keep using `CGWindowListCreateImage` and migrate to the new API later.
Author
Contributor

Another alternative is to use the callback based API NSColorSampler I described in #111303 (comment), but even that may have some caveats (described in that comment).

Another alternative is to use the callback based API `NSColorSampler` I described in https://projects.blender.org/blender/blender/issues/111303#issuecomment-1068185, but even that may have some caveats (described in that comment).
Member

At the end I updated the impl to use CGWindowListCreateImage instead, and it works correctly for me, even with the secondary extended displays. But do note that CGWindowListCreateImage got deprecated in macOS 14 (Sonoma) but it's still available - the recommendation is to use ScreenCaptureKit API instead but SCScreenshotManager (what we need) is only available in macOS 14+ & it's callback based.

We currently support back to 10.15 (Catalina) on Intel, 11 (Big Sur) on Apple Silicon. So it will be quite a while before we can rely on something as new ScreenCaptureKit.

I would just go with what you have and we can revisit later. Even if we have to add parallel code for the different versions.

Let's make a new build.

> At the end I updated the impl to use `CGWindowListCreateImage` instead, and it works correctly for me, even with the secondary extended displays. But do note that `CGWindowListCreateImage` got deprecated in macOS 14 (Sonoma) but it's still available - the recommendation is to use ScreenCaptureKit API instead but [`SCScreenshotManager`](https://developer.apple.com/documentation/screencapturekit/scscreenshotmanager) (what we need) is only available in macOS 14+ & it's callback based. We currently support back to 10.15 (Catalina) on Intel, 11 (Big Sur) on Apple Silicon. So it will be quite a while before we can rely on something as new ScreenCaptureKit. I would just go with what you have and we can revisit later. Even if we have to add parallel code for the different versions. Let's make a new build.
Member

@blender-bot package macos

@blender-bot package macos
Member

@blender-bot package macos

@blender-bot package macos
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR115187) when ready.
Member

Buildbot is having some issues that the moment. Did manage to build without errors on ARM, but is failing some tests unrelated to this PR.

Will try again later.

Buildbot is having some issues that the moment. Did manage to build without errors on ARM, but is failing some tests unrelated to this PR. Will try again later.
Member

@blender-bot package macos

@blender-bot package macos
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR115187) when ready.
Member

@blender-bot package macos

@blender-bot package macos
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR115187) when ready.
Rajesh Malviya force-pushed eyedrop-mac from 7442892cee to d5303d90a0 2023-12-06 21:50:00 +01:00 Compare
Author
Contributor

Rebased to main, if it makes any difference.

Rebased to main, if it makes any difference.
Member

@blender-bot package macos

@blender-bot package macos
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR115187) when ready.
Author
Contributor

@Harley seems like the build succeeded this time.

@Harley seems like the build succeeded this time.
Member

@rajveermalviya - seems like the build succeeded this time.

Awesome!

I've added a comment to that BlenderArtist thread for the one tester, who reported multi-monitor issues, to try again.
https://blenderartists.org/t/testing-requested-for-mac-eyedropper-builds/1494179/7

> @rajveermalviya - seems like the build succeeded this time. Awesome! I've added a comment to that BlenderArtist thread for the one tester, who reported multi-monitor issues, to try again. https://blenderartists.org/t/testing-requested-for-mac-eyedropper-builds/1494179/7
Brecht Van Lommel approved these changes 2023-12-11 19:48:12 +01:00
Brecht Van Lommel left a comment
Owner

Implementation looks good to me.

Implementation looks good to me.
Author
Contributor
@Harley, looks like now it works correctly for them. :) https://blenderartists.org/t/testing-requested-for-mac-eyedropper-builds/1494179/8
Member

@Harley, looks like now it works correctly for them. :)

Yes, that is the same person who commented on the BlenderArtists thread - I made that one there during the cyberattack. But this might be all we get for feedback for now. So you plus two others might have to do.

This isn't very potentially harmful in that this code isn't even called when selecting from a blender window, so I might just make do a build looking for any compiler warnings or formatting issues then commit if fine.

> @Harley, looks like now it works correctly for them. :) Yes, that is the same person who commented on the BlenderArtists thread - I made that one there during the cyberattack. But this might be all we get for feedback for now. So you plus two others might have to do. This isn't very potentially harmful in that this code isn't even called when selecting from a blender window, so I might just make do a build looking for any compiler warnings or formatting issues then commit if fine.
Harley Acheson added 1 commit 2023-12-13 19:47:20 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
eaaf94f3a1
Merge branch 'main' of projects.blender.org:blender/blender into pr115187
Member

@blender-bot build macos

@blender-bot build macos
Harley Acheson merged commit 639de68aaa into main 2023-12-13 22:03:05 +01:00
Rajesh Malviya deleted branch eyedrop-mac 2023-12-13 22:04:36 +01: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 project
No Assignees
5 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#115187
No description provided.