Support for item model in holograms #341
Conversation
d0by1
left a comment
d0by1
left a comment
There was a problem hiding this comment.
Hello, I'd like to see the following changes implemented before I merge.
Can you also describe how you tested this, and what Minecraft versions you tested it on, please?
| return itemModel; | ||
| } | ||
|
|
||
| public static float extractCustomModelData(ItemStack itemStack) { |
There was a problem hiding this comment.
This method is unused, so remove it.
| return new ItemNbtData(extractItemModel(nbt), extractCustomModelData(nbt)); | ||
| } | ||
|
|
||
| public static String extractItemModel(ItemStack itemStack) { |
There was a problem hiding this comment.
This method is unused, remove it.
| @@ -1,12 +1,16 @@ | |||
| package eu.decentsoftware.holograms.hook; | |||
|
|
|||
| import com.google.gson.Gson; | |||
| return extractCustomModelData(NBT.itemStackToNBT(itemStack)); | ||
| } | ||
|
|
||
| public static float extractCustomModelData(ReadWriteNBT nbtItem) { |
| return extractItemModel(NBT.itemStackToNBT(itemStack)); | ||
| } | ||
|
|
||
| public static String extractItemModel(ReadWriteNBT nbtItem) { |
| modifiableNBT.mergeCompound(originalNBT); | ||
|
|
||
| // Load the nbt data | ||
| ItemNbtData nbtData = ItemNbtData.fromJson(nbt); |
There was a problem hiding this comment.
At line 50, the value of nbt is used to parse NBT and merge the resulting compound with modifiableNBT. Considering the value of nbt can be a JSON instead of actual NBT, won't that just fail or go wrong?
I also don't like how one variable nbt is used for two different things. It can either be NBT or your JSON with some data that will be added to NBT. Can you somehow separate these?
When you separate them, you may not even need the JSON at all. You could just use the ItemNbtData object in both HologramItem and here, which is much nicer in my opinion.
There was a problem hiding this comment.
You bring up a good point. This is something I got stuck on as I was trying to think of a good way to integrate this into the current data fixer solution you use for custom model data.
Here's what I found: When the minecraft:item_model component is passed through the datafixer, it gets mapped under the minecraft:custom_data component instead of being left alone. This breaks the display of the item since the minecraft:item_model component needs to be at the root level similar to CustomModelData. Since this component did not exist in 1.20.4, the data fixer treats it as custom data. That's why I parse the NBT data back into JSon and manually add the component after running the data fixer.
I can make a modification to see how this would work without the data fixer.
|
I've made the changes to the pull request. Could you review them and let me know any feedback? |
2 participants
The new 'minecraft:item_model' component allows you to change the model of an item. This PR adds support for items with this component.