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

Fix drops #563

Merged
merged 36 commits into from
Apr 26, 2022
Merged

Fix drops #563

merged 36 commits into from
Apr 26, 2022

Conversation

darksunlight
Copy link
Contributor

@darksunlight darksunlight commented Feb 13, 2022

  • adds basic support for enchantments
  • fixes item drops (visually) (1.15+)
  • implements more accurate drops based on tools (types, enchantments) used (1.14+)

@rom1504 rom1504 added this to Needs triage in PR Triage Feb 20, 2022
@rom1504
Copy link
Member

rom1504 commented Mar 8, 2022

what's the status here ?

@rom1504 rom1504 moved this from Needs triage to Waiting for user input in PR Triage Mar 8, 2022
@darksunlight
Copy link
Contributor Author

what's the status here ?

I just have to

  • replace my temporary selector parser
  • implement check for version for entity metadata
  • implement check for enchantment max level and compatibility

@darksunlight darksunlight marked this pull request as ready for review March 9, 2022 19:27
src/lib/plugins/players.js Outdated Show resolved Hide resolved
src/lib/plugins/players.js Show resolved Hide resolved
@@ -34,7 +34,10 @@ async function read (uuid, spawnPoint, worldFolder) {
pitch: playerData.Rotation.value.value[1],
onGround: Boolean(playerData.OnGround.value)
},
inventory: playerData.Inventory.value.value
inventory: playerData.Inventory.value.value.map(nbtItem => {
if (nbtItem.tag) nbtItem.tag.name = ''
Copy link
Member

Choose a reason for hiding this comment

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

Why

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nbtItem.tag.name would be undefined otherwise and would cause an error on join.

Copy link
Member

Choose a reason for hiding this comment

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

if it's defined and not empty you're losing the value
but it sounds like the problem should be fixed wherever the error was happening instead of here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nbtItem.tag.name is intended to be empty. It's just somehow lost during the saving process, and I don't know prismarine-nbt enough to figure out what caused it to omit this empty string value.

Copy link
Member

Choose a reason for hiding this comment

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

can you do if (nbtItem.tag && nbtItem.tag == undefined)

Copy link
Member

Choose a reason for hiding this comment

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

did it

@@ -16,7 +16,7 @@ module.exports.server = (serv, { version }) => {
const itemPlaceHandlers = new Map()
serv.placeItem = (data) => {
const handler = itemPlaceHandlers.get(data.item.type)
return handler ? handler(data) : { id: data.item.type, data: data.item.metadata }
return handler ? handler(data) : (serv.supportFeature('theFlattening') ? {} : { id: data.item.type, data: data.item.metadata })
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong

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 I understand what you mean.

Copy link
Member

Choose a reason for hiding this comment

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

why would we return nothing after the flattening ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

placeItem is currently only used in the same file, handling the block_place packet from clients. It's destructured into id and data after being called, then the id is used to find the corresponding blocks. Blocks with the same ID as an item is not the corresponding block for the item when the item does not have a corresponding block post-flattening, so I return an empty object with ID undefined such that the wrong block won't be placed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In pre-flattening times however this can be used as a fallback for items that somehow don't have an item place handler as the old assumption is indeed true.

@rom1504 rom1504 closed this Mar 19, 2022
PR Triage automation moved this from Waiting for user input to Closed Mar 19, 2022
@rom1504 rom1504 reopened this Mar 19, 2022
@rom1504
Copy link
Member

rom1504 commented Mar 19, 2022

Sorry misclicked

@rom1504
Copy link
Member

rom1504 commented Mar 19, 2022

This PR is a collection of random stuff, some seems clearly good and some not obvious
What do you think about opening some more specific PRs instead?

That or explaining more what is each of the 10 changes supposed to do ?

@darksunlight
Copy link
Contributor Author

darksunlight commented Mar 19, 2022

@rom1504
Here's a breakdown of the changes:

File name Description
package.json increase mocha timeout to resolve randomly failing CI tests and bump minecraft-data for up-to-date data
playerDat.js changes to player.dat saving and retrieving to support enchantments
commands.js addition of @s selector (for manual testing with /enchant command)
digging.js fix item drops
login.js read item data properly for 1.13+ to support NBT (including enchantments)
logout.js for changes in playerDat.js
placeBlock.js prevent items with no corresponding blocks from being placed (this bug annoyed me a lot when testing item drops)
players.js 1. added convenient function to get players given a selector string for use in /enchant
2. Fix /gamemode command (annoying bug that hindered testing)
3. Change /give to use the new convenient function
4. Add the /enchant command
spawn.js spawn items with correct metadata thus fixing their visual appearance
tabComplete.js misc changes to facilitate testing

@@ -215,6 +215,7 @@ module.exports.server = function (serv, { version }) {
const pos = opt.pos
let sample
if (type === 'all') sample = serv.players
else if (type === 'self') sample = serv.players.filter(p => p.id === selfEntityId)
Copy link
Member

Choose a reason for hiding this comment

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

this needs to be added in doc

@@ -287,17 +288,18 @@ module.exports.server = function (serv, { version }) {
else return sample.slice(count) // Negative, returns from end
}

serv.selectorString = (str, pos, world, allowUser = true) => {
serv.selectorString = (str, pos, world, allowUser = true, ctxEntityId) => {
Copy link
Member

Choose a reason for hiding this comment

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

add in doc

@rom1504
Copy link
Member

rom1504 commented Apr 26, 2022

I'm going to merge.

If we get reports of things being broken due to that, I will revert, and the next steps will be to create many small PRs doing one unitary changes for this instead of this massive PR.

@rom1504 rom1504 merged commit 80928f7 into PrismarineJS:master Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
PR Triage
  
Closed
Development

Successfully merging this pull request may close these issues.

None yet

3 participants