Skip to content

Fix placeable/destory keys deserialization from spigot#6706

Closed
Machine-Maker wants to merge 1 commit into
PaperMC:masterfrom
Machine-Maker:fix/itemstack-deserialization-from-spigot
Closed

Fix placeable/destory keys deserialization from spigot#6706
Machine-Maker wants to merge 1 commit into
PaperMC:masterfrom
Machine-Maker:fix/itemstack-deserialization-from-spigot

Conversation

@Machine-Maker
Copy link
Copy Markdown
Member

Fixes #6689

@Machine-Maker Machine-Maker requested review from a team as code owners October 2, 2021 21:56
@blablubbabc
Copy link
Copy Markdown

blablubbabc commented Oct 3, 2021

Somethings else to consider regarding the representation of those tags in Bukkit: Minecraft stores these tags in a list (duplicates are not filtered, order is preserved), whereas this CraftMetaItem implementation seems to put them into an unordered HashSet. So a back and forth conversion from Minecraft -> Bukkit -> Minecraft might result in those tags being reordered, potentially affecting the comparisons of Minecraft items. Maybe it makes sense to store these tags in a List in Bukkit as well. Or at least store them in a LinkedHashSet. Or enforce a particular order, similar to how enchantments are reordered in Paper.

@Machine-Maker Machine-Maker force-pushed the fix/itemstack-deserialization-from-spigot branch from 7656c95 to 3018cf5 Compare October 4, 2021 23:50
@Machine-Maker
Copy link
Copy Markdown
Member Author

Somethings else to consider regarding the representation of those tags in Bukkit: Minecraft stores these tags in a list (duplicates are not filtered, order is preserved), whereas this CraftMetaItem implementation seems to put them into an unordered HashSet. So a back and forth conversion from Minecraft -> Bukkit -> Minecraft might result in those tags being reordered, potentially affecting the comparisons of Minecraft items. Maybe it makes sense to store these tags in a List in Bukkit as well. Or at least store them in a LinkedHashSet. Or enforce a particular order, similar to how enchantments are reordered in Paper.

Yeah, I just handled that by changing the fields on CraftMetaItem to lists, but the API still returns a set.

Comment thread patches/server/0267-Implement-an-API-for-CanPlaceOn-and-CanDestroy-NBT-v.patch Outdated
@Machine-Maker Machine-Maker force-pushed the fix/itemstack-deserialization-from-spigot branch from 3018cf5 to 3048c34 Compare October 16, 2021 02:48
@stale
Copy link
Copy Markdown

stale Bot commented Dec 15, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@Machine-Maker Machine-Maker force-pushed the fix/itemstack-deserialization-from-spigot branch from 3048c34 to 671e394 Compare December 15, 2021 04:43
@stale stale Bot removed the resolution: stale label Dec 15, 2021
@Machine-Maker
Copy link
Copy Markdown
Member Author

Rebased for 1.18.1

@stale
Copy link
Copy Markdown

stale Bot commented Feb 13, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@Machine-Maker Machine-Maker force-pushed the fix/itemstack-deserialization-from-spigot branch from 671e394 to 6216b78 Compare February 13, 2022 20:22
@stale stale Bot removed the resolution: stale label Feb 13, 2022
@stale
Copy link
Copy Markdown

stale Bot commented Apr 18, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@Machine-Maker Machine-Maker force-pushed the fix/itemstack-deserialization-from-spigot branch from 6216b78 to 59609a5 Compare April 22, 2022 01:27
@stale stale Bot removed the resolution: stale label Apr 22, 2022
@Machine-Maker
Copy link
Copy Markdown
Member Author

Rebased for 1.18.2

@stale
Copy link
Copy Markdown

stale Bot commented Jun 22, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale
Copy link
Copy Markdown

stale Bot commented Jul 2, 2022

This issue has been automatically closed because it has not had activity in a long time. If the issue still applies to the most recent supported version, please open a new issue referencing this original issue.

@stale stale Bot closed this Jul 2, 2022
@Machine-Maker Machine-Maker added status: accepted Disputed bug is accepted as valid or Feature accepted as desired to be added. status: rebase required and removed resolution: stale labels Jul 3, 2022
@Machine-Maker Machine-Maker reopened this Jul 3, 2022
@Machine-Maker
Copy link
Copy Markdown
Member Author

Not applicable anymore

@Machine-Maker Machine-Maker deleted the fix/itemstack-deserialization-from-spigot branch July 16, 2024 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: accepted Disputed bug is accepted as valid or Feature accepted as desired to be added. status: rebase required

Projects

Status: Closed

Development

Successfully merging this pull request may close these issues.

CanDestroy and CanPlaceOn tags from serialized item stacks are lost when switching between Spigot and Paper.

4 participants