Add flower pot implementation

[ghost]

For this pull request is this pull request for Glowkit required: Glowkit#48. This basically adds all the necessary implementation for it.

Modifications Add the implementation, as block state, for org.bukkit.block.FlowerPot introduced in the previously mentioned pull request. This is the only class that distinguishes the pre-1.7 way of handling flower pot contents (block data) and the post-1.7 way (tile entities). This implementation is also now registered to the item table. Apart from that a flower pot tile entity and block type implementation are added.

Result Proper past, present and future proof handling of flower pots.

---

Edited by turt2live

Related Links:

• Glowkit: GlowstoneMC/Glowkit#48
• Ticket: #325

[turt2live]

GLOWKIT: 48

[turt2bot]

@turt2live This PR has been associated with GlowstoneMC/Glowkit#48 - Add flower pot state

[turt2live]

@alexspieksma Your PR has a problem:

1. Place a flower pot on the ground
2. Attempt to place a sand block on top of the empty pot

• It will show for a moment but disappear

3. Place a sapling in the pot

• A sapling will appear above and inside the pot

[ghost]

@turt2live The issue listed under 2) is due to the fact that there's no filter for interacting with the pot with invalid items. This is commented on, on line 58 in BlockFlowerPot.java. What actually happens is that the client thinks the sand block is placed on top of the pot, while the server handles it as putting the sand block in the pot. I decided not to implement this check, since these kind of checks for e.g. inventories, such as brewer inventories, are also not included yet.

The problem under 3) is a little bit more interesting. In this case the pot is already filled, since the sand block has been put in and thus false is returned in BlockFlowerPot#blockInteract(GlowPlayer, GlowBlock, BlockFace, Vector). As a result the sapling is placed, by the server, on the side of the pot you clicked on, because apart from that, Glowstone currently has no (or nearly no) implementation for BlockType#canPlaceAt(GlowBlock, BlockFace) and relies heavily on the client instead. The sappling also appears in the pot, because TileEntity#update(GlowPlayer) hasn't been overridden as explained in the previous comments. Causing the client to not know there's a sand block in the pot and rendering a sapling in it.

[turt2live]

I do not think that this should be accepted without proper implementation then. This does not mean going around fixing all of the existing logical components, but it does mean that flower pots should function correctly. If the resulting system can be potentially carried over to other logical components in the game then the fix should be designed with that in mind.

[ghost]

@turt2live Alright. I'll include checks for valid pot contents and override TileEntity#update(GlowPlayer) in TEFlowerPot after https://github.com/GlowstoneMC/Glowstone/pull/267 has been pulled, because it contains the necessary components for sending tile entity changes to clients. However, being able to place plants on the sides of pots seems unrelated to this pull request and more like something that should be fixed separately. Signs, for example, can also be placed on, in vanilla, invalid blocks. Although adding the aforementioned features may "fix" this placement issue. What do you think?

[turt2live]

The issue of placing plants on the sides of the pot is not the concern as it is an unintended side effect problem. Resolving the concern that flower pots can contain invalid contents will more than likely resolve other concerns relating to invalid block placement.

PRHOLD: Waiting for #267 - Requires entity change code

[turt2live]

Hold cleared: Skulls have been pulled. Pending further review.

[ghost]

@turt2live After implementing TileEntity#update(GlowPlayer) in TEFlowerPot and some testing, it seems like the client handles all the valid contents checks before sending a packet to the server. So is including checks for valid pot contents still necessary in BlockFlowerPot#blockInteract(GlowPlayer, GlowBlock, BlockFace, Vector), in this pull request?

[turt2live]

Yes, because the client may not be the Mojang Vanilla client. In general, assume that the client is not following expected behaviour and bullet-proof everything that the client sends. I know Glowstone doesn't do much of this as-is, but going forward Glowstone will be doing so. Existing code in Glowstone will be improved to match.

[ghost]

@turt2live Should be complete now!

[turt2live]

This has several issues with loading flower pots created in other worlds.

1.8 test

Vanilla

This PR

1.7.10 test

Vanilla

This PR

Please fix!

[turt2live]

@alexspieksma Your PR does not compile

[ghost]

@turt2live I made a few modifications and tested them by importing a vanilla 1.7.10 and 1.8.1-pre3 flatworld, containing all valid pot contents in flowerpots, into Glowstone. Everything worked as far as I could tell, although there seems to be one problem. All flowerpots are visually empty - only visually because pointing at the pots shows the correct contents in the f3-menu - and after placing a block near them, the contents appears. I'm not sure what causes this oddity though.

By the way, the current code converts pots that previously used material IDs in their NBT data to pots that use material ID names. Vanilla does this as well.

[turt2live]

Sounds like you need to send block updates.

[ghost]

@turt2live Actually, it's a problem with all tile entities using messages for visual updates: see https://github.com/GlowstoneMC/Glowstone/issues/547.

[turt2live]

@alexspieksma any update on this? It's been pending changes for a while

[ghost]

@turt2live I fixed the piece of code you previously commented on. However, flower pots will still seem buggy (being invisible on join from time to time) due to https://github.com/GlowstoneMC/Glowstone/issues/547 still requiring a fix.

[turt2live]

@alexspieksma This pull request failed to compile, please resolve the issue as soon as possible.

[turt2live]

~~@alexspieksma This still doesn't function for a simple case of saplings (didn't test past that). The saplings appear on top of the pots and inside the pots, but logging out and back in proves the pots to be empty (checked via a plugin as well).~~

Edit: I didn't even test to with this PR pulled...

[turt2live]

@alexspieksma disregard my last comment, I never actually tested the PR by accident ._.

However, 1.7.10 -> 1.8 conversion failed as shown below:

1.7.10:

1.8 (Glowstone):

[ghost]

@turt2live Try placing a block next to one of the flowerpots. When I encountered the same issue as displayed in your screenshots this resulted in the pots updating client-sided and showing the correct contents. I'm confident this has to do with https://github.com/GlowstoneMC/Glowstone/issues/547 - in both cases the client does know the correct contents (check the f3 menu while pointing at a pot), but somehow refuses to display it visually.

[turt2live]

I can't consider this complete unless it works as expected though. If that means it needs another implementation to be present, then this can be put on hold.

[turt2live]

@alexspieksma any comment on my most recent response?

[ghost]

@turt2live If you regard this as not working as expected because visually changeable tile entities don't appear properly on join, then putting this pull request on hold until https://github.com/GlowstoneMC/Glowstone/issues/547 is fixed is the only viable option.

[turt2live]

The issue I'm having is that I can't reproduce the tile entities issue outside of this PR at all. I've tried the steps on the mentioned ticket with no success. When I have a lot more time I will be testing this and the mentioned issue even further (although I've already sunk 3x the normal amount of testing into this PR). Steps to reproduce the issue 100% of the time (or more than 0%, really) would be appreciated.