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

Codechange: Disable pointer locking by default for Emscripten #9191

Merged
merged 1 commit into from Jun 28, 2021

Conversation

@embeddedt
Copy link
Contributor

@embeddedt embeddedt commented May 4, 2021

Motivation / Problem

See #9150.

Description

This PR changes the default scroll mode so that pointer locking is not used by default, resolving the bad UX for the Emscripten port.

As this is a breaking change and needs further discussion, it's currently a draft.

Limitations

  • This is a breaking change.

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')
@nielsmh
Copy link
Contributor

@nielsmh nielsmh commented May 4, 2021

There are two kinds of "decorated" mouse cursors. The basic are single-sprite cursors, those are most of the construction tools. In the original graphics the construction tool is integrated in the cursor itself:

image

In OpenGFX the tool appears to be a separate sprite next to the cursor, but it's actually still a single sprite:

image

When you drag a vehicle in a depot, the vehicle graphics is used for the mouse cursor instead, the cursor is still only a single sprite then.

Loading

@PeterN
Copy link
Member

@PeterN PeterN commented May 4, 2021

How does that relate to whether the cursor is locked in position or not when scrolling?

Loading

@embeddedt
Copy link
Contributor Author

@embeddedt embeddedt commented May 4, 2021

@PeterN I think @nielsmh is referring to my comment under Limitations about the cursor being drawn incorrectly.

@nielsmh Hmm... I see what you mean. In that case, I think there will need to be an Emscripten-specific code path added for the cursor rendering, as the only way I know of to change the cursor appearance without having OpenTTD draw it itself is to use CSS.

Loading

@nielsmh
Copy link
Contributor

@nielsmh nielsmh commented May 4, 2021

I think having an alternate cursor code path would be useful regardless, as there are some requests to support a high-contrast cursor style. In my mind that would be basically skipping the baseset for the cursor graphics and draw something custom instead.

Loading

@embeddedt
Copy link
Contributor Author

@embeddedt embeddedt commented May 4, 2021

I've discovered that adding cursor: none !important to the canvas CSS in os/emscripten/shell.html will actually override Emscripten's cursor logic and force the software-rendered OpenTTD cursors to always get drawn. That is probably a good enough compromise for now.

The side effect is that the cursor rendering will now be dependent on OpenTTD's frame rate, but that's also how it works on the normal ports. On my machine I don't see any noticeable lag while testing it.

Loading

@embeddedt
Copy link
Contributor Author

@embeddedt embeddedt commented May 11, 2021

I'm going to mark this as ready for review as I haven't noticed any bugs while testing it.

Loading

@embeddedt embeddedt marked this pull request as ready for review May 11, 2021
@embeddedt
Copy link
Contributor Author

@embeddedt embeddedt commented Jun 7, 2021

@TrueBrain There were some valid concerns raised about changing the default scrolling mode in #9150.

So, is there a way of conditionally choosing the default using the preprocessor inside settings.ini?

Not sure if the ongoing refactoring enables/disables that.

Loading

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jun 7, 2021

Check for stuff like APPLE to see how that works, but yes, you can change defaults for settings depending on #defines. The question of course is: do we want that in this case :D

Loading

@embeddedt embeddedt force-pushed the emscripten_no_pointer_lock branch 2 times, most recently from f819611 to ccda075 Jun 7, 2021
Copy link
Member

@TrueBrain TrueBrain left a comment

After some self-reflection, I came to the realisation the only reason I am against it, is because I don't play the game like that :P But that is a bullshit reason. And I agree, the web-version would be 10x better if it doesn't constantly grab the mouse pointer .. so yeah, let's do this, and see where it takes us :)

(to be clear, my request for change is to make sure this is only changing the default for the web-version. Not for any of the other versions ;) ).

Loading

src/table/settings/settings.ini Show resolved Hide resolved
Loading
src/video/sdl2_v.cpp Outdated Show resolved Hide resolved
Loading
src/video/sdl2_v.cpp Outdated Show resolved Hide resolved
Loading
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jun 28, 2021

Looks good to me. If you wouldn't mind rebasing and squashing your commits to a single one, that would make the commit checker happy too :D Tnx!

Loading

@embeddedt embeddedt force-pushed the emscripten_no_pointer_lock branch from 59c8794 to b6e4564 Jun 28, 2021
@embeddedt embeddedt force-pushed the emscripten_no_pointer_lock branch from b6e4564 to 5a87d45 Jun 28, 2021
@embeddedt embeddedt changed the title Codechange: Disable pointer locking by default Codechange: Disable pointer locking by default for Emscripten Jun 28, 2021
Copy link
Member

@TrueBrain TrueBrain left a comment

LGTM! Tnx!

For who-ever merges, please change Codechange to Change. Sure, everything is a codechange in the end, but this is a bit more than that :D

No need to push a new version for it, we can fix it on squash :)

Loading

@TrueBrain TrueBrain merged commit 883e4ea into OpenTTD:master Jun 28, 2021
15 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants