Layer moved while painting with brush

[DiodorS]

A layer is moved while painting with brush, if while the paint stroke is being drawn I press Ctrl key (on Windows). Note, that I never have Ctrl pressed while clicking (and/or holding) the mouse to switch to the move tool. Probably the keyboard modifier key event is processed out of order, or the brush stroke code tests for Ctrl being pressed in real time, not at the time the stroke was scheduled.

Steps to reproduce:

1. Create a big RGB, 16-bit document (4240x6059), no background, one empty layer

2. Create a new layer, paint on something to notice layer shifts (for example the text "test")

3. Duplicate that layer

4. Set soft brush, size 2375, spacing 1% so that painting the stroke takes some time

5. Quickly draw several long strokes using mouse or tablet, to put them in a queue

6. While the stroke is being printed, let go of the mouse, press & hold Ctrl on the keyboard.

7. Observe: subsequent queued paint strokes are executed as Move commands (bug)

8. Repeat the above, but without holding Ctrl - all paint strokes are executed after several seconds (intended behavior)

Same issue with 'Eraser" tool.

See the attached video. I've included OSD with key/mouse presses to show, that I never press Ctrl while painting the strokes. I press Ctrl after I let go of the mouse.

Severity: medium/high, it happens very rarely, but when it happens and the layer is moved without me noticing it, I have to redo some work if I merge the layers and have not enough history to undo. 

CC: @CJButler (who solved the "stuck space bar" issue, thx!)

Please let me know if you can replicate this issue.

Versions affected (Windows):

non-beta: Adobe Photoshop Version: 25.11.0 20240716.r.706 b326a7d x64

beta: Adobe Photoshop Version: 25.12.0 20240722.m.2708 7e0e5ce x64

[CJButler]

Good info and Steps to Reproduce. Thanks. Logged a bug, PS-137040.

But I have to be honest, it's probably not going to get ranked very high by the Product Managers unless a lot of people complain, despite the impact on your workflows. It's probably been this way for several major releases.  A Smaller Brush will reduce the lag, as might changing the Memory Tile sizes (not sure). It also depends on the Brush Tip Style; some complex brushes are lag machines when they are made big. It will take a major overhaul to change that behavior.

One thing that might be done is to somehow reject KB modifiers while a brush stroke is still painting. That code a very fussy, so we'd have to be very careful how we did that. We'd almost certainly upset someone else by changing that behavior without a great deal of care. So: High cost, which is also considered when evaluating what to fix.

[DiodorS]

@CJButler thanks for logging the bug!

The lag of a big brush is to be expected and I have no issue with that.

> One thing that might be done is to somehow reject KB modifiers while a brush stroke is still painting.

Yes, that might solve this particular issue, but would not address the root cause.

I've tried sampling a color (by Alt+clicking, releasing Alt immediately) while the stroke was being painted. That revealed another strange behavior. A single dot is being painted in the place where I sampled a color from, as if I just clicked there, without holding the Alt key.

On the other hand, when I paint a "slow" stroke, then click without holding Alt, and press & hold Alt after clicking, a color is sampled instead of painting a dot in the place I clicked.

I can only speculate how the code works, but it seems like the KB modifiers are read using GetAsyncKeyState (which ignores the message queue) to decide what tool operation should be scheduled (brush, move, sample color), whereas mouse/pen events are processed from the event queue. If the modifiers keys were also read from the event queue (using GetKeyState, without Async), then the state of modifier keys should be updated appropriately for the tool choice. It should solve those odd behaviors, but I guess it is not that simple...

[DiodorS]

Well... it was that simple 🙂 At least it seems to solve both problems I've described and is not breaking anything obvious.

I've hot-patched GetAsyncKeyState (in user32.dll) to jmp directly to GetKeyState. This is quite heavy-handed, but it's a good place to start. There are many justified uses of the Async versions for sure, and patching them all willy-nilly will break something else. But if you can pinpoint the call used in the drawing/scheduling code path, that might be it 🙂

[CJButler]

Good job - and interesting. I've added your comments and findings to the bug report. The bug will be will be reviewed next Wednesday.

As you might guess, the Photoshop Event Handling implementation is ... complex. Probably more than you can imagine. It has layers and layers of implementations applied and patched over the years. It requires support for multiple platforms (Mac, Windows, etc.). It has an unfortunate mix of an Event Queue pump, Paint Brush Stroke smoothing, real time hardware polling (we've worked hard to minimize this, but it is still required in places), Event Queue Peeking, and more. We've talked about rewriting the whole system, but our assessment is that it will be an extraordinary effort, and it will likely have a whole new set of bugs that will take years to find and fix.

One of the more useful bits of input you can provide between now and next Wednesday is your honest assessment of how this negatively impacts your day-to-day workflow, and whether your workarounds are tolerable or not.

I'll add your comments to the bug. Thanks again!

[DiodorS]

@CJButler Thanks! I'm very grateful for you quick and honest responses. I'm curious, if this issue happens also on non-Windows platforms. I'm not even trying to imagine the complexity and layers that grew over the years. I'm myself very, very, surprised that ditching the Async version solved the problem on my end. I will use the patched version for a while and report any issues. But I use a very specific subset of functionalities (no artboards, no stroke smoothing, etc.) and this change might have impact that is hard to predict, even with access to the source code.

> how this negatively impacts your day-to-day workflow,

It's manageable. But the original issue (with moved layer) happens ~2x day. I have to redo some work ~x1 month, as I usually notice the layer shift, and have enough history to undo, but it's irritating.

> and whether your workarounds are tolerable or not

I'm OK with running the patched file, but every new release (especially beta releases that happen very often), would require me to create a new patch. Time consuming, but better than having layers randomly moved.

Adding to my previous post, I did a little more digging. Fortunately `GetKeyState` is present in the imports table. I was able to find a single call in Photoshop binary that has to be patched. It's a very small stub, called from >50 other places, but at least I don't have to patch a system-level dll.

If you are interested in trying my workaround, here is the patch (you can use https://hexed.it/) for Photoshop.exe:

For "Adobe Photoshop Version 25.11.0 20240716.r.706 b326a7d x64", file offset 0x00A303C6, change 96 to D6

For "BETA Adobe Photoshop Version 25.12.0 20240722.m.2708 7e0e5ce x64", file offset 0x27927D6, change 3EB0 to CEAF

(all it does is changing a call to GetAsyncKeyState into a call to GetKeyState)

[CJButler]

Thanks - I'll add your comments to the bug and they will be seen and discussed during the review.

I think if we took your approach concept, we'd do it at the source level. I understand it well enough. The unintended ripple effects are hard to foresee when we consider all the workflows of our larger user base, so whatever we do, we will proceed with great caution.

[DiodorS]

@CJButler thanks!

I would be grateful if you would consider, as a last resort, changing to the non-Async version guarded by a flag in PSUserConfig.txt (let's say: `UseNonAsyncKBModifiers 1`) . That way only the bravest of the users would be exposed to potentially breaking change. I'm keeping my fingers crossed for the review!

[CJButler]

@DiodorS  So we have this issue Open in our bug backlog. This weekend, I took a look at what it would take to sort it out. Unfortunately a blanket change to GetKeyState is not a safe thing to do. There are about 50 instances of direct calls to GetKeyState and GetAsyncKeyState, about evenly mixed, and from a casual survey of those calls, it's clear the original authurs had reasons for using deliberately one function or the other. Furthermore, there are ~ 6 additional code paths that abstract GetKeyState or GetAsyncKeyState into generic functions which are then abstracted into various utility function that are used all over the code base. So the implications of making such a change are unknown and risky. Things like using the ecsape key to cancel gestures may stop working, for example.

I then looked into seeing if I could spot the specific code that trapped the ctrl key while the laggy brush waas painting, and while I can see the WM_KEYDOWN event, I cannot see which functions are calling GetAsyncKeyState while the paint stroke is being generated without adding a bunch of debugging code. 

I have to get back to my assigned tasks, so I'll have to put this issue on hold for a while. The bug will go back into out backlog with some notes. I had hoped it would be easy to do, but it is not looking that way. I'll update this thread when I find some more time.