Skip to content
This repository has been archived by the owner on Nov 9, 2017. It is now read-only.

Add new title-based inventory methods. Adds BUKKIT-1899 #54

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Aaron1011
Copy link

Original Bukkit PR: Bukkit#1110


Bukkit currently lacks a way to set the title of an inventory through its API, forcing those who wish to implement such a functionality to use (already existing) NMS methods.

@Aaron1011
Copy link
Author

@turt2live: This should be marked as 'Missing Glowstone'

@gabizou
Copy link

gabizou commented Nov 11, 2014

Aren't named inventories achieved through the name of the entity of which they belong to?

@Aaron1011
Copy link
Author

@gabizou: This is for things like chests, which can have different names displayed in the inventory title.

@turt2live
Copy link

@Aaron1011 As a warning: Any changes made to the PR will be left under @mproncace's name when/if pulled.

As for the content...
This is missing the following in order to be even slightly pulled:

  • Documentation on what happens when an inventory title is changed with a player currently looking at it
  • A strong use case justification to expose an implementation detail
  • A strong use case justification to consider this not a bug in the protocol
  • A better way to indicate a "nameable" inventory
  • Documentation formatting (Glowkit still follows many of the contributing guidelines left from Bukkit)

@Aaron1011
Copy link
Author

A strong use case justification to expose an implementation detail

This is alreay possible

A strong use case justification to consider this not a bug in the protocol

This is certainly not a protocol bug - From the Minecraft Wiki page: A chest renamed using an anvil will display its custom name when placed as a block.. This also works for dispensers.

A better way to indicate a "nameable" inventory

What kind of way? A method on InventoryType?

@turt2live
Copy link

@Aaron1011 Renaming a chest is of course a feature, but renaming a chest that is in block form? That seems like an implementation detail and a bug in the protocol. (There's a lot of missing CraftBukkit comments regarding this PR).

Considering not everything is nameable, a separate interface would likely be best.

@Aaron1011
Copy link
Author

@turt2bot: Look at the Open Window packet. When you rename a chest in vanilla, the server just stores the new name, and sends it in the packet.

@turt2live
Copy link

Yes, but one of the concerns is that this is an unintentional feature of the protocol and not a supported one, much like setting the number of arrows visible on the player.

@Aaron1011
Copy link
Author

@turt2live: I think I see what you mean. There needs to be a way to set the inventory name before it gets opened - a method of InventoryHolder, maybe

@turt2live
Copy link

No, there needs to be evidence that setting the name of a block is supported by Mojang.

@Aaron1011
Copy link
Author

@turt2live: I have no idea what you mean. This is just setting the name of an inventory, not a block.

@turt2live
Copy link

Yes, but that inventory is linked to the block through the tile entity. So I guess my phrasing should be changed to tile entity, not block.

@Aaron1011
Copy link
Author

@turt2live: Okay, I see what you mean. The name is only used for inventory titles. I don't think there's any reason why is shouldn't be changeable.

@turt2live
Copy link

I'm not arguing the feature, I'm arguing whether or not this is intended functionality in the Minecraft protocol.

Let's say this is pulled, implemented, and all is well. And then Minecraft X.Y comes along and all of a sudden this is no longer possible. We're falsely saying that you can still do this.

Of course this scenario can be applied to other parts of the API, but this seems to rely on a bug in the protocol/design of the game, much like making items glow.

@turt2live
Copy link

I should also clarify that the commit will be in @mproncace's name, but an honourable mention will be given to @Aaron1011 in the commit message (or in the author list) for work on the PR, if this is pulled.

@caseif
Copy link

caseif commented Nov 11, 2014

I'll take this opportunity to point out that modifying the name of an inventory is most definitely not a protocol bug; the original accompanying implementation simply changed the value of a String in net.minecraft.server.Inventory which was sent to the client the next time the inventory as a whole was resent.

@turt2live
Copy link

Yes, but my concern is that the feature introduced is forced. The proposed feature may very well be removed from the game as it may be an unintentional side effect of the protocol.

I'd like to see/read some kind of evidence that this is going to be here for a long while before moving forward with it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants