-
-
Notifications
You must be signed in to change notification settings - Fork 944
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 gamemode test and implementation #3508
Merged
extremeheat
merged 6 commits into
PrismarineJS:1.21.3
from
IceTank:fix-gamemode-test-and-implementation
Dec 11, 2024
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
e346813
Fix gamemode test and implementation
IceTank cd66986
Add gamemode test related comments
IceTank c362658
Fix gamemode tests
IceTank e3a0158
Add gamemode out of bounds checks
IceTank 6fb4b5d
Simplify gameMode parsing and check against spawnRespawnWorldDataFiel…
IceTank 0edf070
Merge branch '1.21.3' into fix-gamemode-test-and-implementation
IceTank File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,29 @@ | ||
// test to see if bot retains creative gamemode in bot object on death | ||
|
||
const assert = require('assert') | ||
const { onceWithCleanup } = require('../../lib/promise_utils') | ||
|
||
module.exports = async (bot) => { | ||
bot.test.becomeCreative() | ||
bot.chat(`/kill ${bot.username}`) | ||
await new Promise((resolve, reject) => setTimeout(resolve, 5000)) | ||
assert.strictEqual(bot.game.gameMode, 'creative', 'Failed to parse respawn packet') | ||
module.exports = () => { | ||
const tests = [] | ||
|
||
function addTest (name, f) { | ||
tests[name] = (bot) => f(bot) | ||
} | ||
|
||
addTest('change', async (bot) => { | ||
await bot.test.becomeSurvival() | ||
assert.strictEqual(bot.game.gameMode, 'survival', 'Wrong gamemode after switching gamemode') | ||
await bot.test.becomeCreative() | ||
assert.strictEqual(bot.game.gameMode, 'creative', 'Wrong gamemode after switching gamemode') | ||
}) | ||
|
||
addTest('after respawn', async (bot) => { | ||
await bot.test.becomeCreative() | ||
bot.test.selfKill() | ||
await onceWithCleanup(bot, 'respawn', { timeout: 5000 }) | ||
// Respawn packets send the gamemode. If the bot is in creative mode, it should respawn in creative mode. Tested <1.20 | ||
assert.strictEqual(bot.game.gameMode, 'creative', 'Wrong gamemode after respawn') | ||
}) | ||
|
||
return tests | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 understand how this is different from the current code. It does the same thing.
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.
Pre 1.20.5, the respawn packet and the login packet used numbers to indicate what gamemode the player is in. The problem is that with one packet, it's
gamemode
, and with the other, it'sgameMode
.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.
packet.gamemode
could be a number. But game.gameMode needs to be a string. Also,||
is wrong as it will be falsy if the gamemode is 0. And 0 is a valid gamemode in some versions where the packet has gamemode as a number.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 line was broken before
bot.game.gameMode = packet.gamemode || parseGameMode(packet.gameMode)
. It only works correctly in versions after 1.20.5. In versions pre 1.20.5, it would set game.gameMode to a number for gamemodes other than survival. No one noticed that because most people use survival mode and the test was not working, and the change was only made recently.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.
gamemode
lower case is only defined on 1.20.5+ because this function is called with a SpawnInfo object.gameMode
is the previous number field.||
can be replaced with??
, but otherwise for the else condition it should be this:parseGameMode
can likely just be set togameModes[gameModeBits] || 'survival']
I think it may be worthwhile to just backport this change inside minecraft-data to 1.20.5 to all the old versions so we can get rid of the logic here
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.
No, the two different cases have been used before. Handle respawn is used by the respawn packet and the login packet. They both use different cases for the same field pre 1.20.5.
See https://prismarinejs.github.io/minecraft-data/?v=1.12.2&d=protocol#toClient_respawn
And
https://prismarinejs.github.io/minecraft-data/?v=1.12.2&d=protocol#toClient_login
The same protocol uses different versions of the field gamemode.
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 think it's better to focus on the tests instead of the implementation. The protocol is inconsistent and changes a lot across versions. Trying to figure out what the implementation should look like by looking really hard at the code always fails in mineflayer. That's why there are tests. They tell you if something works or not. The current implementation does not work for pre 1.20.5. Your changes to game.js to update past 1.20.5 only when through review because the gamemode test was not working and did not catch the bug with the different cased gamemode fields.
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.
Yes what I mean is there are things we can fix in minecraft-data to avoid the need to do special handling in mineflayer.
Can you reopen the PR against the 1.21.2 branch?