Skip to content

Added blockstate data support for furnaces.#479

Merged
PseudoKnight merged 5 commits intoEngineHub:masterfrom
Pieter12345:furnace_nbt_data_support
Apr 7, 2018
Merged

Added blockstate data support for furnaces.#479
PseudoKnight merged 5 commits intoEngineHub:masterfrom
Pieter12345:furnace_nbt_data_support

Conversation

@Pieter12345
Copy link
Copy Markdown
Contributor

Furnaces will now have a "burntime", "cooktime" and "inventory" metadata tag. "inventory" is an array with 3 possible keys: "result", "fuel" and "smelting".
This PR prevents set_pinv(pinv()) to erase furnace metadata and adds the ability to get/modify it.

Furnaces will now have a "burntime", "cooktime" and "inventory" metadata tag. "inventory" is an array with 3 possible keys: "result", "fuel" and "smelting".
This commit prevents set_pinv(pinv()) to erase furnace metadata and adds the ability to get/modify it.
@PseudoKnight
Copy link
Copy Markdown
Contributor

If we add this, we might as well add all inventory blocks. Furnace seems an odd first choice. I never once pick-blocked a furnace.

https://hub.spigotmc.org/javadocs/spigot/org/bukkit/block/Container.html

@Pieter12345
Copy link
Copy Markdown
Contributor Author

On my redstone server, furnaces are used as variable signal strength input to comparators. As soon as they are moved in a players inventory using CommandHelper, they lose their data.
Implementing this per container block gives the advantage of having tags for the inventory slots that correspond with the actual NBT tags (as used in the vanilla /give command). I agree that this should eventually be done for all container blocks, but I didn't want to spend my time on that.

The now supported containers are: BrewingStand, Chest, Dispenser, Dropper and Hopper.
@Pieter12345
Copy link
Copy Markdown
Contributor Author

Added support for other containers as well, tested this for chests, hoppers and droppers.

Fixup for the previous 2 commits.
Brewing stands only had support for fuel and ingredient meta, this adds support for the 3 remaining bottle slots.
Due to these objects being wrapped into a BukkitMCItemStack, they are never null. So check for empty item stacks instead.
@PseudoKnight
Copy link
Copy Markdown
Contributor

I tested it on some local servers and everything seems to be working as one would expect on 1.12.2 and 1.8.8. BlockStateMeta didn't exist in 1.7.10 and Brewing Stands BlockStateMeta was fixed in 1.11 but doesn't seem to cause any problems prior to that.

I did notice that you followed my pattern of only including slots that existed in shulker boxes. However, with named keys I wonder if this is still appropriate. (eg. furnaces and brewing stands) I think it might make more sense to include the keys but set them to null in those cases. I could even be convinced to have all keys in containers be set to null too. I think I was concerned with rapidly ballooning inventory arrays with the introduction of shulker boxes. See, normally in the meta we include the key if it exists for that item meta. (eg. "display" always exists because all item meta supports it... book "title" always exists but only for books even if a title doesn't exist) What do you think?

@Pieter12345
Copy link
Copy Markdown
Contributor Author

@PseudoKnight Good to hear that it works on previous versions too. Regarding always creating indices, I actually prefer an array_index_exists(@data, 'key') over @data['key'] == null to check whether a property exists or not. Having many keys with empty arrays and nulls assigned to them makes it more annoying to read for debugging purposes (while developing), it costs a bit more memory where it doesn't have to and it doesn't actually add more information. The only benefits I see is that a developer can assume certain keys exist (but would still have to check for null) and that you can see which keys could be altered by printing the info (as opposite to setting a property first and then printing info to see which key was added with which value).
As for consistency, I chose to always include an "inventory" array for items which have an inventory, but only add keys of existing items to that array to not make the data too spammy.

Since we're talking about metadata anyways, I would very much prefer method MCItemMeta itemMeta(Construct, MCMaterial, Target) to throw an Exception or print a warning when supplying one or multiple unused keys. Currently, warnings are only printed for invalid (non-integer or out of range) inventory keys. I would prefer a warning on every unused key to give more feedback to the developer about which supplied keys are not used, rather than the developer noticing that his code isn't performing what he had in mind without feedback. What are your thoughts on that?

@PseudoKnight
Copy link
Copy Markdown
Contributor

I've seen people use unused keys before, so I'm not sure how I feel about that. It'll also have an overhead. In any case, I think this is good to go.

@PseudoKnight PseudoKnight merged commit 0fcd35e into EngineHub:master Apr 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants