ItemStack and ItemStackSnapshot inconsistent in toContainer implementations

[pie-flavor]

Custom data is entirely not present in the UnsafeData tag. An example from an in-development plugin:

{
    "ContentVersion": 1,
    "ItemType": "minecraft:armor_stand",
    "Count": 1,
    "UnsafeDamage": 0,
    "Data": [
      {
        "ContentVersion": 2,
        "ManipulatorId": "armorstandtools:tool_data",
        "ManipulatorData": {
          "ContentVersion": 1,
          "ToolType": "SUMMON"
        }
      }
    ],
    "UnsafeData": {
      "SpongeData": {
        "CustomManipulators": [
          {
            "ContentVersion": 2,
            "ManipulatorId": "armorstandtools:tool_data",
            "ManipulatorData": {
              "ContentVersion": 1,
              "ToolType": "SUMMON"
            }
          }
        ]
      },
      "display": {
        "Lore": [
          "R-Click to summon an armor stand and pick it up",
          "Any click will drop it"
        ],
        "Name": "Summon Armor Stand"
      }
    }
  }

ItemStackSnapshot data

  {
    "ContentVersion": 1,
    "ItemType": "minecraft:armor_stand",
    "Count": 1,
    "UnsafeDamage": 0,
    "UnsafeData": {
      "display": {
        "Lore": [
          "R-Click to summon an armor stand and pick it up",
          "Any click will drop it"
        ],
        "Name": "Summon Armor Stand"
      }
    },
    "Data": [
      {
        "ContentVersion": 2,
        "ManipulatorId": "armorstandtools:tool_data",
        "ManipulatorData": {
          "ContentVersion": 1,
          "ToolType": "SUMMON"
        }
      }
    ]
  }

ItemStack data

[JBYoshi]

I get the exact opposite result. For me, the ItemStack has the manipulators only in the Data tag, and the ItemStackSnapshot has the manipulators in both Data and UnsafeData.

I'm assuming it would be better to have the data only in Data instead of duplicating it in UnsafeData. If so, I've got a patch ready to go.

[pie-flavor]

@JBYoshi You have gotten an identical result while labeling an "opposite result". And the point of UnsafeData is to get the data as it would go to NBT, right?

[gabizou]

The reason why we prune in UnsafeData is to reduce duplication. ItemStackSnapshot is somewhat outdated from those efforts, in that sense they should be transitive with regards to their data being identical. Only difference is one being mutable and one being immutable.

Good catch. Come 1.13 we're likely to re-evaluate how Data serialization is handled (and of course write some solid converters for old ways to new ways, might take a while).

@JBYoshi if you have a fix, make it match ItemStack where manipulators are not included in UnsafeData as that is where they should not exist in our toContainer() implementation (as of current contract). If possible with the patch, it should bump the content version for whichever implementation is being modified to properly prune old data.