Skip to content

Add a fullscreen button for the game div container #2855

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

Merged
merged 21 commits into from
Apr 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
177c86b
Add a fullscreen button for the game div container
Johnwz123 Mar 19, 2024
2031975
Merge branch 'master' into feat/game-fullscreen
sayomaki Mar 20, 2024
0832d8b
Merge branch 'master' into feat/game-fullscreen
RichDom2185 Mar 21, 2024
af140dc
Merge branch 'master' into feat/game-fullscreen
martin-henz Mar 23, 2024
d9e3a2e
Merge branch 'master' into feat/game-fullscreen
RichDom2185 Mar 24, 2024
31fc2c9
Merge branch 'master' into feat/game-fullscreen
RichDom2185 Mar 24, 2024
1a5b3a9
Modify code to use the useFullscreen hook from the @mantine/hooks pac…
Johnwz123 Mar 25, 2024
eef4ae8
Merge branch 'master' of https://github.com/source-academy/frontend i…
Johnwz123 Mar 25, 2024
f9e4c3e
Merge branch 'feat/game-fullscreen' of https://github.com/source-acad…
Johnwz123 Mar 25, 2024
5ffcabe
Merge branch 'master' into feat/game-fullscreen
martin-henz Mar 26, 2024
7ec8ad3
Merge branch 'master' into feat/game-fullscreen
sayomaki Mar 26, 2024
76e09cb
Merge branch 'master' into feat/game-fullscreen
martin-henz Mar 28, 2024
744b8b3
Bump dependencies
RichDom2185 Mar 28, 2024
3c1a46d
Use `IconNames` instead of magic strings
RichDom2185 Mar 28, 2024
4b50bab
Merge branch 'master' into feat/game-fullscreen
RichDom2185 Mar 28, 2024
daac858
Merge branch 'master' into feat/game-fullscreen
RichDom2185 Mar 30, 2024
e41fd96
Merge branch 'master' into feat/game-fullscreen
lhw-1 Apr 1, 2024
a0d28d1
Merge branch 'master' into feat/game-fullscreen
martin-henz Apr 1, 2024
4f4718e
Merge branch 'master' into feat/game-fullscreen
martin-henz Apr 2, 2024
e2b6672
Merge branch 'master' into feat/game-fullscreen
RichDom2185 Apr 6, 2024
9c6b6bf
Merge branch 'master' into feat/game-fullscreen
RichDom2185 Apr 10, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"@blueprintjs/icons": "^5.5.0",
"@blueprintjs/popover2": "^2.0.0",
"@blueprintjs/select": "^5.0.0",
"@mantine/hooks": "^7.7.0",
"@octokit/rest": "^20.0.0",
"@reduxjs/toolkit": "^1.9.7",
"@sentry/browser": "^7.57.0",
Expand Down
89 changes: 88 additions & 1 deletion src/pages/academy/game/Game.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
import { Icon } from '@blueprintjs/core';
import { IconNames } from '@blueprintjs/icons';
import { useFullscreen } from '@mantine/hooks';
import React from 'react';
import { useDispatch } from 'react-redux';
import { useTypedSelector } from 'src/commons/utils/Hooks';
Expand Down Expand Up @@ -50,9 +53,93 @@ function Game() {
}
}, [session, achievements, goals]);

// This is a custom hook imported from @mantine/hooks that handles the fullscreen logic
// It returns a ref to attach to the element that should be fullscreened,
// a function to toggle fullscreen and a boolean indicating whether the element is fullscreen
const {
ref: fullscreenRef,
toggle: toggleFullscreen,
fullscreen: isFullscreen
} = useFullscreen<HTMLDivElement>();

// This function is a wrapper around toggleFullscreen that also locks the screen orientation
// to landscape when entering fullscreen and unlocks it when exiting fullscreen
const enhancedToggleFullscreen = async () => {
toggleFullscreen();

if (window.screen.orientation) {
if (!isFullscreen) {
window.screen.orientation.lock('landscape');
} else {
window.screen.orientation.unlock();
}
}
};

const gameDisplayRef = React.useRef<HTMLDivElement | null>(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this gameDisplayRef here, can we not just use the ref provided by the useFullscreen hook?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. The gameDisplayRef is a React RefObject that I used in line 99 to 113 to handle the resizing of the game display when entering/exiting fullscreen. But the ref provided by the useFullscreen hook is a callback ref function. See https://mantine.dev/hooks/use-fullscreen/#definition.

Copy link
Member

Choose a reason for hiding this comment

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

My question is why do you need to handle the resizing manually? Doesn't useFullscreen already handle it for you?

Copy link
Contributor Author

@Johnwz123 Johnwz123 Mar 29, 2024

Choose a reason for hiding this comment

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

I added the fullscreen Icon element as a child of the game-display div element, which the Phaser game engine inserts the game canvas element in. The game canvas element has a fixed aspect ratio and its size is determined by the size of the game-display div element. When browser window width is too wide and height is too small, there will be empty space on the left and right of the game canvas element. So the left offset of the fullscreen Icon element is updated to ensure that it remains aligned with the top left of the game canvas element. The Icon size and padding were purely for aesthetic reasons.

I couldn't think of a better design since the game is a canvas element. To avoid the need for this, perhaps it would be better to implement the fullscreen option directly within the game itself. But I haven't gone into the Phaser game documentation yet so I'm not sure if it's possible.

Copy link
Member

Choose a reason for hiding this comment

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

I think the solution is very hacky. Why not just rely on the DOM to position the icon button correctly instead of using a resize observer? Instead of putting the button as a child you can just put it as a sibling, the wrap a parent container around both.

Maybe something like:

{/* parent, relative, margin auto to squeeze child + align center */}
<div style={{ position: 'relative' }}>
  {/* absolute, top = left = padding */}
  <Icon /* ... */ />
  <div id="game-display"></div>
</div>

Copy link
Member

Choose a reason for hiding this comment

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

Then you don't need to use any resize logic cause the DOM does the layouting for you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried using margin: auto with the Icon button both as a child and as a sibling with a parent container wrapper. However, this causes issues with the resizing of the game canvas element since it references the game-display div element's width and height to determine its width and height. So, when you increase the size of the browser window, more margin gets added instead of the game canvas element getting enlarged.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I understand it now; I still feel the solution is hacky. After looking at game code, I have an idea for a refactor. Let me discuss with @lhw-1 first.


// This function sets the gameDisplayRef and also calls the ref callback from useFullscreen
// to attach the fullscreen logic to the game display element
const setGameDisplayRefs = React.useCallback(
(node: HTMLDivElement | null) => {
// Refs returned by useRef()
gameDisplayRef.current = node;

// Ref callback from useFullscreen
fullscreenRef(node);
},
[fullscreenRef]
);

// Logic for the fullscreen button to dynamically adjust its size, position and padding
// based on the size of the game display.
const [iconSize, setIconSize] = React.useState(0);
const [iconLeft, setIconLeft] = React.useState('0px');
const [iconPadding, setIconPadding] = React.useState('0px');

React.useEffect(() => {
const handleResize = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Please don't put this function definition inside the useEffect body, coupled with the resize observer below it results in so many unnecessary function objects being recreated, degrading performance.

if (gameDisplayRef.current) {
Copy link
Member

Choose a reason for hiding this comment

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

Are the values never reset otherwise?

const aspectRatio = 16 / 9;
const height = gameDisplayRef.current.offsetHeight;
const width = gameDisplayRef.current.offsetWidth;
const size = height / 40;
const padding = height / 50;
const leftOffset =
isFullscreen || height * aspectRatio > width ? 0 : (width - height * aspectRatio) / 2;
setIconSize(size);
setIconPadding(`${padding}px`);
setIconLeft(`${leftOffset}px`);
Comment on lines +103 to +112
Copy link
Member

Choose a reason for hiding this comment

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

I don't get it:

  1. Why are iconSize, iconPadding, iconLeft independent state variables? Aren't they all derived state?
  2. What does the game height and width have anything to do with the icon positioning?
  3. What is the aspect ratio for?

}
};

// When exiting fullscreen, the browser might not have completed the transition
// at the time handleResize is called, so the height of gameDisplayRef.current
// is still the fullscreen height.
// To fix this, we delay handleResize by 100ms.
const delayedHandleResize = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto on moving this out of the useEffect body.

setTimeout(handleResize, 100);
};

window.addEventListener('resize', delayedHandleResize);
delayedHandleResize();

return () => window.removeEventListener('resize', delayedHandleResize);
Comment on lines +124 to +127
Copy link
Member

Choose a reason for hiding this comment

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

What is the resize observer for when we already have isFullscreen as a state variable?

}, [isFullscreen]);

return (
<>
<div id="game-display"></div>
<div id="game-display" ref={setGameDisplayRefs}>
<Icon
id="fullscreen-button"
icon={isFullscreen ? IconNames.MINIMIZE : IconNames.FULLSCREEN}
color="white"
htmlTitle={isFullscreen ? 'Exit full screen' : 'Full screen'}
Copy link
Member

Choose a reason for hiding this comment

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

Why are we using htmlTitle instead of using Blueprint's Tooltip? Is there any particular reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, no particular reason. I was just looking for a way to implement a simple tooltip and came across using htmlTitle first. Will update to use Tooltip instead.

Copy link
Member

Choose a reason for hiding this comment

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

Any updates? I'll merge this PR first but please do change it in the future if you have the time (or create an issue if you don't).

size={iconSize}
onClick={enhancedToggleFullscreen}
style={{ left: iconLeft, padding: iconPadding }}
/>
</div>
{isTestStudent && (
<div className="Horizontal">
<button
Expand Down
6 changes: 6 additions & 0 deletions src/styles/_game.scss
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,9 @@
width: 100%;
align-items: center;
}

#fullscreen-button {
position: absolute;
cursor: pointer;
z-index: 100;
}
5 changes: 5 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1919,6 +1919,11 @@
resolved "https://registry.yarnpkg.com/@leichtgewicht/ip-codec/-/ip-codec-2.0.4.tgz#b2ac626d6cb9c8718ab459166d4bb405b8ffa78b"
integrity sha512-Hcv+nVC0kZnQ3tD9GVu5xSMR4VVYOteQIr/hwFPVEvPdlXqgGEuRjiheChHgdM+JyqdgNcmzZOX/tnl0JOiI7A==

"@mantine/hooks@^7.7.0":
version "7.7.0"
resolved "https://registry.yarnpkg.com/@mantine/hooks/-/hooks-7.7.0.tgz#52f0fdc97e953798d2e632aa5e90959f389cbd1e"
integrity sha512-m99vMzeONMpBLv0Rcb2LD88xAhpvwVdTMBo/7WohBDYtk1shJKHAc/WbQ/cJPcNk11Bzp/mhx/EPNZfs9+NwZA==

"@mapbox/node-pre-gyp@^1.0.0":
version "1.0.10"
resolved "https://registry.yarnpkg.com/@mapbox/node-pre-gyp/-/node-pre-gyp-1.0.10.tgz#8e6735ccebbb1581e5a7e652244cadc8a844d03c"
Expand Down