Fix methods for occupied state in Bed MaterialData

[Johni0702]

The Issue:

Currently there is no way to set whether a bed is occupied. Setting it using Block.setData(byte) does not work either (and is deprecated).

Neither is it possible to do this in Glowstone itself as the setFacingDirection overrides the the occupied flag and the getFacingDirection produces incorrect results due to using a 3 bits mask (& 0x7) instead of the necessary 2 bit mask (& 0x3).

PR Breakdown:

~~This PR adds methods to set and get whether a bed is occupied.~~ It also fixes the getFacingDirection method by using the correct bit mask and the setFacingDirection by modifying the current data value instead of recalculating it. All changes should be backwards compatible.

~~While the new methods introduced can be omitted, the fixes for the other two methods are mandatory for correctly implementing beds in Glowstone.~~

[turt2live]

Setting the bed as occupied should not be possible unless occupied means something other than "there is a player in here"

[dequis]

Setting the bed as occupied should not be possible

I don't get it, why shouldn't it be possible?

[turt2live]

Because a bed should not falsely report that a player is contained within the bed unless an actual player is within the bed. This would also be additional API that would need to be supported and is not clear as intended functionality.

If there is a valid use case for this (that does not have a more optimal solution), I am willing to reconsider.

[dequis]

I see, makes sense. Well, the use case is in the other PR - this is yet another of those "glowstone needs too much of bukkit to be able to do stuff"

[turt2live]

That's not a valid use case. Glowstone should, and can, still operate without a heavy dependency on Glowkit. Although it's easy, it should not be maintained as such.

[dequis]

So, MaterialData#setData from glowstone?

[turt2live]

Without doing heavy review that does appear to be the solution, yes.

[Johni0702]

I can remove the two new methods. As you said they aren't really necessary and might cause unexpected behavior.. MaterialData#setData from Glowstone might work for setting it. However subsequent calls to getFacing will always return BlockFace#EAST and calling setFacingDirection(BlockFace) will reset all changes done manually via MaterialData#setData as it recalculates the data value instead of just modifying it. So using said method is not a solution for this problem (unless the set/getFacing methods are fixed). I do not know how CB handled that. I suspect they might just loop through all players and get their current bed to find out if anyone is occupying a bed. However I do not consider this to be the correct solution as it doesn't set the occupied flag which can even be seen when pressing F3 in the client (not test, will have a closer look at it when I get back home).

[turt2live]

The occupied state should remain read only. If a plugin plays with data values then they can deal with that consequence. The setter also doesn't appear to do anything that can't be done in Glowstone.

[Johni0702]

The occupied state should remain read only [...] The setter also doesn't appear to do anything that can't be done in Glowstone.

I approve that statement and will update the PR accordingly.

If a plugin plays with data values then they can deal with that consequence.

That's correct. However Glowstone has to be able to modify the data value which it currently is not. Imagine the following scenario:

• Player enters bed -> Glowstone code sets the occupied flag
• Plugin tries to find the direction of the bed in order to do some custom stuff -> Bed#getFacing always returns BlockFace#EAST instead of the actual direction
• Assuming the bed was actually facing east and, for whatever reason, the plugin calls Bed#setFacingDirection(BlockFace.EAST): What you'd expect is nothing happening. What actually happens is that the Bed#setFacingDirection method reset the occupied flag which allows yet another player to use the bed.

[turt2live]

The get and set methods don't appear to need to be changed with the removal of the method. The only thing that would happen would be the removal of setOccupied which contains this line of code: setData((byte) (isOccupied ? (getData() | 0x4) : (getData() & ~0x4)));. That line of code can be done in Glowstone.