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

Fix #1620 #1626

Merged
merged 3 commits into from
Nov 14, 2024
Merged

Fix #1620 #1626

merged 3 commits into from
Nov 14, 2024

Conversation

Hoikas
Copy link
Member

@Hoikas Hoikas commented Nov 12, 2024

Fixes #1620 by avoiding a double-scale of mouse coordinates. I also included a bonus multi-monitor fix.

I hardwired the cursor to always think it's hovering over a player with the name It's a Spooky Ghost!...

On master:
image

With this PR:
image

For reference:
image

MSDN says:
Do not use the LOWORD or HIWORD macros to extract the x- and y- coordinates of the cursor position because these macros return incorrect results on systems with multiple monitors. Systems with multiple monitors can have negative x- and y- coordinates, and LOWORD and HIWORD treat the coordinates as unsigned quantities.
The OS is already giving us the correct coordinates. No need to scale
them.
Copy link
Member

@dpogue dpogue left a comment

Choose a reason for hiding this comment

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

I suspect this will need testing on macOS retina screens /cc @colincornaby

@@ -492,7 +492,7 @@ bool plMouseDevice::MsgReceive(plMessage* msg)
fXPos = pXMsg->fX;

SetCursorX(fXPos);
fWXPos = pXMsg->fWx * fScale;
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 broke macOS. I'll dig into why. Just for background - why didn't this work on Windows?

Copy link
Member Author

Choose a reason for hiding this comment

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

WM_MOUSEMOVE and friends send coordinates that are already DPI aware, therefore there is no need to scale them. They are already scaled.

Copy link
Contributor

Choose a reason for hiding this comment

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

That explains it. The input system on macOS does not scale with DPI. Let me consider what to do (including possibly scaling the macOS input by hand) and I'll get back to you shortly.

@colincornaby
Copy link
Contributor

Suggested change for macOS, in Sources/Plasma/Apps/plClient/Mac-Cocoa/PLSView.mm

    NSRect windowViewBounds = self.bounds;
    CGFloat xNormal = (windowLocation.x) / windowViewBounds.size.width;
    CGFloat yNormal =
        (windowViewBounds.size.height - windowLocation.y) / windowViewBounds.size.height;

    plIMouseXEventMsg* pXMsg = new plIMouseXEventMsg;
    plIMouseYEventMsg* pYMsg = new plIMouseYEventMsg;
    
    pXMsg->fX = xNormal;
    pYMsg->fY = yNormal;
    
    // Plasma internally uses input coords as display coords
    pXMsg->fWx = viewLocation.x * self.window.screen.backingScaleFactor;
    pYMsg->fWy = (windowViewBounds.size.height - windowLocation.y) * self.window.screen.backingScaleFactor;

    @synchronized(self.layer) {
        if (self.inputManager) {
            self.inputManager->MsgReceive(pXMsg);
            self.inputManager->MsgReceive(pYMsg);
        }
    }

@Hoikas
Copy link
Member Author

Hoikas commented Nov 14, 2024

@colincornaby I've pushed your change and an additional two to ensure that scaled coordinates are going down when the mouse is being recentered. I've verified that the code compiles, but the client login window doesn't actually seem to appear on my system. Maybe this is just a problem with my setup or an unrelated regression, but I'd appreciate your taking a glance at things.

@colincornaby
Copy link
Contributor

I am seeing the login window. Weird.

I've seen the login window not appear when the client can't connect to the game server (it just gets stuck in a retry loop, might be its own bug.) When the login window doesn't appear you could try pausing in the debugger to see where it is stuck.

Otherwise, this change looks good on my machine.

@Hoikas
Copy link
Member Author

Hoikas commented Nov 14, 2024 via email

@Hoikas Hoikas merged commit 61175a8 into H-uru:master Nov 14, 2024
17 checks passed
@Hoikas Hoikas deleted the fix_1620 branch November 14, 2024 23:08
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.

Names of the people are far away
3 participants