API change: OnPlayerPlacingBlock / OnPlayerPlacedBlock

[xoft]

I'm currently working on a change that requires a change to the signature of the OnPlayerPlacingBlock and OnPlayerPlacedBlock hooks. Up until now the hooks were only being called as a direct response for the rclk packet, providing all the values from the packet as their parameters. However, I want to alter the logic behind the hooks to be "Any block (being) placed by the player, even indirectly", which means liquids placed by buckets, both blocks of the doors etc. These indirect placements don't have all the params that the hooks require. I don't think the hooks need those parameters anyway, and if they did, the plugin would process the rclk hook instead. So I'd like to change the signatures from:

function OnPlayerPlacingBlock(Player, BlockX, BlockY, BlockZ, BlockFace, CursorX, CursorY, CursorZ, BlockType, BlockMeta)
function OnPlayerPlacedBlock(Player, BlockX, BlockY, BlockZ, BlockFace, CursorX, CursorY, CursorZ, BlockType, BlockMeta)

to:

function OnPlayerPlacingBlock(Player, BlockX, BlockY, BlockZ, BlockType, BlockMeta)
function OnPlayerPlacedBlock(Player, BlockX, BlockY, BlockZ, BlockType, BlockMeta)

(losing the cursor and blockface parameters)

I've done a quick survey of current plugins and they don't seem to use the parameters anyway:
EssentialsSpawn ( https://github.com/tonibm19/EssentialsSp.../hooks.lua ):
- only uses the OnPlayerPlacingBlock hook and only the block coords
- will continue working without a change, although the param names won't match
Core ( https://github.com/mc-server/Core/blob/m...eblock.lua ):
- only uses the OnPlayerPlacingBlock hook and only the block coords. The face is used but only to ignore non-place operations (those won't trigger the new hooks at all)
- will need to be changed with the new API
Commandy ( https://github.com/mc-server/Commandy/bl...y_main.lua )
- only uses the OnPlayerPlacingBlock hook and only the blocktype
- will need to be changed to work with the new API
HungerGames ( https://github.com/mc-server/HungerGames.../Hooks.lua )
- plugin already defunct
- only uses the OnPlayerPlacingBlock hook and only the blocktype
- will need to be changed to work with the new API
tAuthentication ( https://github.com/bearbin/tAuthenticati...r/main.lua )
- only uses the OnPlayerPlacingBlock hook and none of the params
- will continue working without a change, although the param names won't match
PrivateBlocks ( https://github.com/madmaxoft/PrivateBloc...rState.lua )
- only uses the OnPlayerPlacingBlock hook, and only the block coords
- needs no change for the new API

This change will allow me to unify the block-placing code into a single function in the cPlayer class, even export it to Lua so that plugins can use it to impersonate players while placing blocks. It will fix these issues:
- https://github.com/mc-server/MCServer/issues/1618
- https://github.com/mc-server/Gallery/issues/45
- Players can place and scoop water and lava in other players' areas despite the Gallery plugin (not reported on GitHub yet)

The API breakage is in my opinion minimal and easily fixable for existing plugins. What do you think?

[NiLSPACE]

Makes sense. I think it's a good change.

[NiLSPACE]

Re-thinking about it. Why remove the blockface parameter?

[xoft]

Because there's no blockface when placing the additional blocks. Consider the scenario of placing a door:
1. Client sends the BlockPlace packet
2. Server parses the packet, calls the packet-based hook (PLAYER_RIGHT_CLICK) with all the params
3. Server calls the door item handler to handle the placement
4. The door itemhandler calls PLAYER_PLACING_BLOCK hook for the lower part of the door (what the client sent)
5. The door itemhandler calls PLAYER_PLACING_BLOCK hook for the upper part of the door <-- There's no blockface available here!

The situation is quite similar with placing and scooping up liquids. There's no blockface for these in the packets.

The hook should simply inform that "the player is placing a block at these coords". If a plugin needs the blockface, it will need to process the lower packet - PLAYER_RIGHT_CLICK.

[NiLSPACE]

Then we could send BLOCK_FACE_NONE or calculate it ourself.

[xoft]

I don't think making up data that's not there is any good. What if it's a special-shaped block and the server "guesses" the face wrong? And what good is BLOCK_FACE_NONE if it's a constant value passed to the hook?

You're wanting the blockface param rather badly, can you explain the scenario you need it for?

[NiLSPACE]

I can't think of one, but I'm sure someone could use it somewhere.
Maybe we can add another constant called BLOCK_FACE_UNKNOWN?

[xoft]

I still think that if a plugin needs the blockface, it is interested more in the raw packet than the actual block placement, so the PLAYER_RIGHT_CLICK hook is better suited for that.

I'd like to hear a third opinion on this. Anyone?

[sphinxc0re]

I think you're right. A hook for block-placement is really only about the block, the player, and the position of the block. The cursor thing belongs to the right_click hook.

[xoft]

If I'm counting right, that's three agains one. STR, can you live with the changes, or are you absolutely positive that you need the cursor and blockface?