-
Notifications
You must be signed in to change notification settings - Fork 419
Don't allow pausing online multiplayer games #8478
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
Conversation
addons/pause/check-online.js
Outdated
|
|
||
| function onlineFeaturesDetected() { | ||
| hasOnlineFeatures = true; | ||
| document.querySelector(".pause-btn").classList.add("disabled"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could wait until the user tries to pause before changing the UI. Less controversial 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the button would get disabled when the user tries to click it? That could work if it displays a helpful message ("this project can't be paused because ..."), otherwise it would be really weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could wait until the user tries to pause before changing the UI. Less controversial 😄
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the point of letting the user know we disabled project pausing before they actually try to pause.
I bet many scratchers haven't considered they can use the pause button to cheat and will discover that only because we're telling them by making the pause button gray. Counterproductive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, didn't think about that.
- Handle receiving messages with batches of changes - Fix race condition with pause button element
| const names = Object.keys(cloudVariables); | ||
| const values = Object.values(cloudVariables); | ||
|
|
||
| const varsHaveInvalidNumbers = values.some((i) => String(Number(i)) !== i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this condition might trigger false positives if a cloud variable's value is an actual number instead of a numeral string? (because a string is never strictly equal to a number). not sure if it's intentional or not though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the strict inequality sign is on the outside of the String() constructor?
string(number) !== string
And this function takes in a string as an argument, so it's a string-to-string comparison. …That is, unless Cloud variable value messages aren't always a string. I think they are, but I'll have to check that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is, unless Cloud variable value messages aren't always a string
if a project sets a cloud variable to a number itself (not receiving a change) then it is a number
|
Not sure if this PR already considers that, but what if a user is in the editor and cloud variables are disabled? |
|
i think im heavily against this to be honest.. i dont think its scratch addons’ place to say whether projects can be paused. this is an anti feature. maybe would be useful to make auto pause not break cloud games, but i dont like the idea of adding in all this complexity/opaque behavior. is there any evidence of the pause button being abused/this protection being needed? can’t this be solved on the game developers side checking for some sort of keep alive ping? do we absolutely need to apply these heuristics? it feels really risky for low reward to be honest. |
Yeah, I understand that. The main inspiration for this was the thought of auto pause pausing projects that aren't meant to be paused, like online multiplayer games, (and that was actually what I was initially going to do but then realized it would be a 2-in-1 PR) but I may have taken it too far with this PR. Oh well, at least we have the groundwork now. I'll play around with it.
Agreed. |
It could, but I don't think people should have to worry about patching their projects to prevent this — Scratch Addons has never been part of the Scratch ecosystem. It's just an unofficial browser extension. Unless people still code in pause detectors anyway because users can (although it's impractical) pause projects without Scratch Addons by putting their computer to sleep. |
|
For me, the main issue is the risk for false positives. Maybe we should add a mechanism that allows to bypass the pause prevention after showing a warning to the user. |
|
Also, correct me if I'm wrong because I just skimmed the code, but a popup telling users that pause is disabled would be better than just greying out the button. Maybe also add a "Report a problem" button inside that popup to allow people pausing anyways. |
It's not just users trying to cheat. IIRC Google Chrome and other major browsers set tabs to sleep to save resources, which leads to projects being paused. As Jeffalo pointed out, if you want to make a stable multiplayer game, you must take mitigations anyways. Multiplayer projects need a user inactivity counter to know when players leave, so they can free up cloud variables for other players to join. Also, and I don't know if someone has already considered this, the 60FPS addon can also be used to cheat. Should the algorithm limit that as well? |
|
I still think it makes sense to use this to prevent auto-pause on multiplayer projects. |
So can turbo mode. |
|
What can you really use to cheat? Just float in the air I guess. But also 60 FPS can be used to cheat better since it can speed up your player if the movement isn't using delta time. Turbo mode can also be used to go super fast. I've never actually seen any cheaters or hackers in cloud games before though (except for one), so it's pretty uncommon. |
i do know of one instance where someone used 60fps to cheat in a cloud game once |
|
New pull request: #8555 |
|
This is inching into DRM and client-side anti-cheat, so please don't merge unless there's an easy way to turn it off. |
|
Don't worry, I'm not planning to carry out this change. Similar logic is used in #8555, but that only affects automatic pausing when the window is unfocused, where it makes sense. |
Addresses:
pause: Automatically pause project when the page is not focused #5838 (review)Changes
Added an algorithm that is used to determine whether a project is okay to pause, based on metadata and runtime activity analysis. Basically, if it thinks the project is a multiplayer game, the pause function will be disabled.
Characteristics and patterns that are considered:
Reason for changes
Pausing a game that connects with other players can cause issues and mess up their sessions unless the project was designed to explicitly handle players momentarily disconnecting. That is unfair, so this change solves this part of the problem.
Tests
Tested all logic on several different projects.
Status
This addition hasn't been extensively discussed or had an issue open; the problems were only briefly mentioned on a pull request with related changes, so this implementation may not be final. Do not merge the pull request until a conclusion has been reached.
If this pull request doesn't make it, I'll consider integrating it with an auto-pause feature like #5838, since us pausing an online game would be our fault, not the user's fault.