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

[QoL] Automatically center window vertically when playing on landscape #1114

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

torranx
Copy link
Contributor

@torranx torranx commented May 18, 2024

PLEASE REFER HERE FOR UPDATED DESCRIPTION


This PR includes changes from 851

Only added the check if window width < 1920 - this should prevent applying the align-items: center property when the game window is scaled up.

There is another cleaner approach to achieving this through phaser but this would require a reload. If forcing a reload is okay, then that approach is better, otherwise this is.

  • width < 1920
Screenshot 2024-05-19 at 5 46 38 AM
  • width >= 1920
Screenshot 2024-05-19 at 5 49 38 AM

@bennybroseph bennybroseph self-assigned this May 19, 2024
@bennybroseph bennybroseph added the enhancement New feature or request label May 19, 2024
@bennybroseph
Copy link
Collaborator

There is another cleaner approach to achieving this through phaser but this would require a reload. If forcing a reload is okay, then that approach is better, otherwise this is.

When would the reload occur?

@torranx
Copy link
Contributor Author

torranx commented May 19, 2024

@bennybroseph
Sorry forgot to mention on PR description. Reload will happen when changing Touch Controls settings, this is because:

  • we need to include the check whether the touch controls is enabled or not
  • phaser does not allow us to update game config during runtime (as per my research), so we must do it before initialization, hence the reload

@Odizinne
Copy link
Contributor

Odizinne commented May 21, 2024

(Complete beginner here)

I'm trying to do the same in my electron app for days.

Setting the alignment to center break the login page. The input fields position will change depending of the current resolution.

Also it introduce overflow above 1080p

This is at 1600p
image

and this is 800p
image

EDIT: Nvm i just tried your branch, and i cannot replicate the overflow issue. It might be on my side.

But i cannot manage to get the window centered with your branch either (touch disabled, display set at 2560x1600)

@torranx
Copy link
Contributor Author

torranx commented May 22, 2024

I guess messing with the styles really is not the way to go. I'll update the PR with my other solution after I'm done with IRL work and the other issue in the game I'm trying to fix. Closing this now, will reopen after PR is updated. Thanks!

@torranx torranx closed this May 22, 2024
@Odizinne
Copy link
Contributor

Odizinne commented May 22, 2024

If that helps, from a user perspective i guess triggering a relaod one time when setting an option is not really an issue, but this is only my opinion

@torranx
Copy link
Contributor Author

torranx commented May 22, 2024

EDIT: Nvm i just tried your branch, and i cannot replicate the overflow issue. It might be on my side.

But i cannot manage to get the window centered with your branch either (touch disabled, display set at 2560x1600)

Oh just saw this, i thought what made the overflow was my changes so I reverted them lol.

You should set Desktop (touch) in your devtools too.

I will add back my changes then reopen the PR. Brb sleeping

@torranx
Copy link
Contributor Author

torranx commented May 23, 2024

Confirming the overflow on login page, this only happens on viewports < 1920. Will update my branch with the phaser solution

Screenshot 2024-05-23 at 3 22 23 PM

@torranx
Copy link
Contributor Author

torranx commented May 23, 2024

Since messing with the CSS messed up a part of the UI (login phase), and the reverted PR #851 also encountered issues on certain viewport sizes, I've decided to update the game window through Phaser config's autoCenter instead - this way we are not trying to have conflicts with how Phaser works.

  • Note: the trade-off here is we have to require reload when changing touch controls setting.
  • Also addtl benefit in doing this is we'd avoid layout shifts when the game is being initialized, and then the styles being loaded.

Tests done were a success but would appreciate other tests as well.

For reference (please take note of the device type on the upper portion of the recordings):

Non-mobile devices

  • Portrait (Changing touch controls has no effect on alignment)
Screen.Recording.2024-05-24.at.5.28.53.AM.mov
  • Landscape (Changing touch controls has effect: Disabled -> centers the game window; Enabled -> no center)
Screen.Recording.2024-05-24.at.5.30.33.AM.mov
  • Desktop (no effects when changing touch control settings)
Screen.Recording.2024-05-24.at.5.40.07.AM.mov

Mobile devices (Current behavior applies - you cannot disable touch controls)

No overflow on login phase also:

Screen.Recording.2024-05-24.at.5.58.21.AM.mov

@torranx torranx reopened this May 23, 2024
@Odizinne
Copy link
Contributor

Odizinne commented May 24, 2024

Following this logic, on desktop with no touch controls shouldnt the window be centered by default

If you have no touch control to show in any situation, i guess it make more sense.

  if (!isMobile() && isLandscape 
		&& (typeof isTouchControlsEnabled === "boolean" && !isTouchControlsEnabled)
  ) {
    return true;
  }
  return false;

image

This is what i get by just removing the && hasTouchscreen() check.

This way window is centered on 16:10 laptops with no touchscreens

@torranx
Copy link
Contributor Author

torranx commented May 24, 2024

Following this logic, on desktop with no touch controls shouldnt the window be centered by default

If you have no touch control to show in any situation, i guess it make more sense.

Yeah you do actually have a point but this was initially introduced to enhance the UX on handheld devices, so desktops were really not put into consideration and since most of larger devices already use 16:9.

I'm open to remove that condition but I'm also considering the UX on the small(assumption) number of players accustomed/conditioned to play with what we have right now. I think we'd only want to remove that condition if we get good traction or a lot of users are calling for that change. I actually asked the community on discord and only got 2 replies, 1 yes for mobile (which we do not want), 1 yes generally lol.

@Odizinne
Copy link
Contributor

Odizinne commented May 24, 2024

I'm open for this, since this does not harm other use cases.

As you said mobile is not affected.

If users with touchscreens decide to not use touch control, then the window gets centered. It seems logic to me that user with no touchscreen get the same behavior. This seems to be the general use case most of the time everywhere else (You watch a 16:9 video on a 16:10 device, you have the video centered by default)

I'm trying to think about case where removing this check is a problem, but i cannot really find one (but i'm not that experimented).

Is there any benefits to not center the window on 16:10 screens without touch screen?

I know 16:10 users are a minority but my ideology is that if something can work for a minority without affecting the experience of the majority, it should probably be implemented

@torranx
Copy link
Contributor Author

torranx commented May 24, 2024

It seems logic to me that user with no touchscreen get the same behavior.

I think this is slightly different from our situation/issue now. If that's the case, we want to center the game automatically on non mobile devices with no touchscreen capabilities and not apply the logic of auto aligning the game window when touch controls are enabled/disabled.

Is there any benefits to not center the window on 16:10 screens without touch screen?

This will entirely depend on user preference.

@Odizinne
Copy link
Contributor

Odizinne commented May 24, 2024

If we're going back to original issue #810, creating an ingame setting for this is not the way to go.

Also this issue was not opened with only handhelds in mind, but i think you're right, those are 2 different issues.

So the final goal would be to center the window on desktop / no touchscreen 16:10 devices, and let the touch control option decide for other touchscreen devices that are not mobile?

Device Touch control setting Result
SteamDeck ON Not centered
SteamDeck OFF Centered
16:10 Laptop with no touchscreen OFF / N.A Centered by default

Is that correct?

Sorry had a bad night i'm a lil slow to understand right now!

@torranx
Copy link
Contributor Author

torranx commented May 24, 2024

If we're going back to original issue #810, creating an ingame setting for this is not the way to go.

We're not creating another in game setting.

So the final goal would be to center the window on desktop / no touchscreen 16:10 devices, and let the touch control option decide for other touchscreen devices that are not mobile?

Yes, but the PR does not address desktop-no touchscreen capability yet.

@Odizinne
Copy link
Contributor

Gotcha, 2 different issues

@torranx
Copy link
Contributor Author

torranx commented May 24, 2024

I'll try and add this on my PR since there is some dependencies present.

@Odizinne
Copy link
Contributor

Thank you for the time taken for explanation

@torranx torranx marked this pull request as draft May 24, 2024 07:48
@torranx
Copy link
Contributor Author

torranx commented May 24, 2024

  • Added auto centering on desktop/non-mobile devices with no touch screen capabilities.

Notice the (Requires reload) label is missing and does not reload when going back from settings- this is also true on mobile devices.

Screenshot 2024-05-25 at 2 50 44 AM

@torranx torranx marked this pull request as ready for review May 24, 2024 19:14
@bennybroseph bennybroseph marked this pull request as draft May 25, 2024 12:36
@torranx
Copy link
Contributor Author

torranx commented May 26, 2024

Please refer to the following comments on what are the changes/tests made:

This is ready for review.

@torranx torranx marked this pull request as ready for review May 26, 2024 20:00
@torranx torranx changed the title [v2] Automatically center window vertically when playing on landscape [QoL] Automatically center window vertically when playing on landscape May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants