PaperMC / Paper

The most widely used, high performance Minecraft server that aims to fix gameplay and mechanics inconsistencies
https://papermc.io/
Other
10.02k stars 2.32k forks source link

Placing book in Lectern fires block place event #9759

Open tal5 opened 1 year ago

tal5 commented 1 year ago

Expected behavior

The JD for the block place event says Called when a block is placed by a player., so either for the event to not fire there or for the JD to be clarified.

Observed/Actual behavior

Whenever you place a book into a Lectern, a block place event is fired.

Steps/models to reproduce

This is using the Debuggery plugin (which is part of Paper's org), let me know if proper Java code is preferred.

Plugin and Datapack List

> plugins
[23:44:43 INFO]: Server Plugins (1):
[23:44:43 INFO]: Bukkit Plugins:
[23:44:43 INFO]:  - Debuggery
> datapack list
[23:46:10 INFO]: There are 2 data pack(s) enabled: [vanilla (built-in)], [file/bukkit (world)]
[23:46:10 INFO]: There are no more data packs available

Paper version

> version
[23:49:40 INFO]: Checking version, please wait...
[23:49:41 INFO]: This server is running Paper version git-Paper-196 (MC: 1.20.1) (Implementing API version 1.20.1-R0.1-SNAPSHOT) (Git: 773dd72)
You are running the latest version
Previous version: git-Paper-194 (MC: 1.20.1)

Other

Some Discord discussion if that's relevant.

Brokkonaut commented 1 year ago

I would prefer to have the javadocs changed to reflect that behavior instead of breaking plugins...

Owen1212055 commented 1 year ago

I disagree, this really does not seem like intended behavior. But at the same time, this event's current implementation makes stuff like that very confusing.

Brokkonaut commented 1 year ago

even if it was not intended, it is the only useful event atm (also for waxing, unwaxing, log stripping,.. - playerinteract does not have the resulting change result so it is quite bad to use for this), so it would break nearly every protection plugin if it is changed.

and if it is documented it can be made intended behavior.

Leguan16 commented 1 year ago

100% agree with brokkonaut here.

While this is - as others have said - not the intended way, it is currently also the only way to track such things. So if we decide to add special events for those cases we should probably not change the old behavior but instead add a note to the BPE that goes like this:

"While this event is also triggered on placing books into lecterns, waxing and striping, it is highly recommend to use the appropriate events to handle such interactions"

Leguan16 commented 1 year ago

I have looked into this for a while and discovered that we can just create a CraftItemStack out of the used item and check if it is an actual block. That should fix the issue with lecterns and copper blocks. But what are we doing now?

Warriorrrr commented 1 year ago

Stuff like this shouldn't really be intended behavior as it makes no sense, new API should be added instead. In this case there's already a PR open that adds an insert book event

Leguan16 commented 1 year ago

There were never talks about whether it should be considered intended behavior or not.

Talks currently are at that point that several people said that the current BlockPlaceEvent should be declared as deprecated due to its inconsistency and to introduce a new event.