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: [Emscripten] Only lock pointer on right click #8654

Closed
wants to merge 1 commit into from

Conversation

embeddedt
Copy link
Contributor

Problem and Description

Currently the Emscripten port forcefully locks the pointer (if not locked already) on the first click. I might just be picky, but I find this to be bad UX as I have to hit Escape whenever I wish to move to another tab or do something else.

This got me to think: why does the pointer need to be locked in the first place? After some experimentation I came to the conclusion that the pointer really only needs to be locked when dragging the viewport map using the mouse. As such, this PR adjusts pointer locking so that it only happens when the user is dragging the viewport, and automatically releases afterwards. The result is a smooth user experience with no locking/releasing visible to the player.

Limitations

Releasing the pointer lock in a browser comes with a major caveat: the OS cursor will reappear at the same location where locking was requested, not the new location. As it turns out, OpenTTD freezes the pointer while dragging the map anyway, so this wasn't a problem. However, it is the reason why this locking mechanism cannot be enabled for left clicks, as those are used to build infrastructure, and the pointer does move while dragging to build those.

Another caveat is that I've assumed the only instance where dragging with the RMB held down is used is viewport movement. If that changes in a future OpenTTD release, this solution will need a rework.

I fully understand if the OpenTTD devs are not interested in this fix due to the limitations or would prefer a different solution. I'm suggesting this mainly because I think it's a net positive experience for players, compared to the current behavior.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@embeddedt
Copy link
Contributor Author

I could be wrong but I think the macOS CI failure is unrelated to this PR.

@Milek7
Copy link
Contributor

Milek7 commented Feb 7, 2021

Another caveat is that I've assumed the only instance where dragging with the RMB held down is used is viewport movement. If that changes in a future OpenTTD release, this solution will need a rework.

That depends on scroll_mode setting.

I think browser might show notification when pointer locking, so doing it for every map scroll is potentially even more annoying?

@embeddedt
Copy link
Contributor Author

embeddedt commented Feb 7, 2021

I think browser might show notification when pointer locking, so doing it for every map scroll is potentially even more annoying?

Interestingly, I discovered that this notification doesn't show up when I use this method. However it does show up with the old method. Maybe the browser only does it if the pointer remains locked after the mouse is released?

EDIT: Tested with the preview. It seems that the notification shows up the first time the page loads, but after you have navigated to another tab or window once and come back, it no longer appears until you reload the page. I think that's not too much spam.

@TrueBrain TrueBrain added the preview This PR is receiving preview builds label Feb 8, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8654 February 8, 2021 07:35 Inactive
@Milek7
Copy link
Contributor

Milek7 commented Feb 8, 2021

Hmm, for me it shows notification every time (Firefox 84.0b4)

@TrueBrain
Copy link
Member

First off, really cool you are looking into this :D

On both on Chrome and Firefox on Windows I keep getting the banner. According to both implementations, this is also what should be happening. So I am somewhat surprised that doesn't happen for you :P Sounds like that is a bug, they will fix sooner or later :)

For me this was also the trade-of: constantly show a nag-banner every time a user pressed the right mouse button, or only do it once, capturing the whole mouse. Honestly, not a big fan of the constant showing of a banner, but it is less annoying than I initially expected.

That said, I do agree with you that capturing the mouse constantly is also far from ideal. Alt+tab doesn't work, but in-game it is common to use Escape for some interactions, which first makes you escape the capturing ... which frustrates me a lot :P

This is one of those problems I would consider required to be fixed before we can put a "Play Now" button on the frontpage. And I don't really have an answer.

We initially also toyed with the scroll-mode; we have 4 of them, of which 2 don't require mouse capturing at all. Maybe good to toy around with those too, see if any of these work better?

That said, with 1 of the 4 scroll-modes you should not be capturing the mouse, I think. Maybe you can look into this, if you knew about the scroll-modes, and if they work as you would expect them to? :)

@embeddedt
Copy link
Contributor Author

I had forgotten about the scroll mode settings, but these are even more ideal. If we could have a different default for Emscripten, pointer lock could be disabled unless the scroll mode explicitly needs it. That would get rid of the annoying warning. This means there would need to be a mechanism for JS to retrieve the current setting. Alternatively, we could simply disable the fixed versions of these settings on Emscripten, and only keep the last two.

I guess the question now is: how many players use the RMB drag feature? Personally, I have always moved the map with arrow keys + shift anyways. I didn't even realize this was a feature till months after I started playing the game. 😆

@TrueBrain
Copy link
Member

Going to close this PR for now. Clearly this solution as-is isn't really an improvement (constant nag-screens :P), and we should be on the look out for better solutions.

I think it is a perfect conversation to have, if switching the rmb mode is a good idea. But best hold in a https://github.com/OpenTTD/OpenTTD/discussions for now I think :) Personally I do not really have an opinion about it one way or the other, as I am pretty sure you can get used to any mode :D But others should pitch in on that first, I think.

Again, really appreciate these kind of PRs!

@TrueBrain TrueBrain closed this Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview This PR is receiving preview builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants