TestPlugin.java
test-plugin-1.0-SNAPSHOT.jar
[SPIGOT-4256] Missing Block Physics call nms.Block Created: 11/Aug/18 Updated: 12/Aug/18 |
|
| Status: | Open |
| Project: | Spigot |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Bug | Priority: | Minor |
| Reporter: | chickeneer | Assignee: | Unassigned |
| Resolution: | Unresolved | Votes: | 1 |
| Labels: | None | ||
| Attachments: |
TestPlugin.java
test-plugin-1.0-SNAPSHOT.jar
|
| Version: | This server is running CraftBukkit version git-Bukkit-a0a27e5 (MC: 1.13) (Implementing API version 1.13-R0.1-SNAPSHOT) |
| Guidelines Read: | Yes |
| Description |
|
Not an API bug report, but a missing Physics event call. Providing a plugin but should not be needed to understand the concept. There is a missing physics event call which minimally causes attached blocks like signs to pop off without calling a physics event for that particular block. A video illustrating this concept with the attached plugin is shown. Where the EventHandler is: @EventHandler public void handle(BlockPhysicsEvent event) { Material type = event.getBlock().getType(); if (type == Material.WALL_SIGN || type == Material.SIGN) { event.setCancelled(true); } } Expected behavior is that the sign would not pop off since I am cancelling all physics events involving signs.
The location of the missing event call is in the nms.Block class; with a method that looks like as follows. public void a(IBlockData iblockdata, GeneratorAccess generatoraccess, BlockPosition blockposition, int i) { BlockPosition.b blockposition_b = BlockPosition.b.r(); Throwable throwable = null; try { EnumDirection[] aenumdirection = EnumDirection.values(); int j = aenumdirection.length; for (int k = 0; k < j; ++k) { EnumDirection enumdirection = aenumdirection[k]; blockposition_b.j(blockposition).d(enumdirection); IBlockData iblockdata1 = generatoraccess.getType(blockposition_b); IBlockData iblockdata2 = iblockdata1.updateState(enumdirection.opposite(), iblockdata, generatoraccess, blockposition_b, blockposition); a(iblockdata1, iblockdata2, generatoraccess, blockposition_b, i); } ... The updateState is where the sign popping occurs. This method is occurs after the ALLOWED physics event of the original broken block. I believe this is sufficient information. And it is probably faster for someone else to add in the event call than me. Not well practiced enough to quickly whip up Pull Requests.
Requested declaration PPS: Bug reports which do not 1) contain a declaration of testing in Vanilla and without plugins, or 2) in the case of plugin API bugs, contain a minimal reproduction case (source + jar please) will be closed. Bug reports must contain step by step instructions to reproduce the bug from a clean server install with no assumptions or prior knowledge. Also make sure you include the full output of /version in your report. Please copy and paste this statement to the bottom of your report to indicate that you have read and understood it. |
| Comments |
| Comment by md_5 [ 12/Aug/18 ] |
|
I've updated the docs of the event in the interim. The physics event is a horrible event, and I really don't think adding more calls is the answer, at least if you remotely care about performance. If we take a reasonably sized server of 250 players, and say they manage less than 10% of my contrived example, that's still over 2 million event calls per minute. |
| Comment by chickeneer [ 11/Aug/18 ] |
|
I concede that x6 is an unfair representation to server performance. But, it is a behavior change when compared to 1.12. Perhaps a javadoc update to the BlockPhysicsEvent which describes this case? Essentially stating that a BlockPhysicsEvent may update the state of an immediately adjacent block without a new BlockPhysicsEvent being called. Changing the spec of the event as such seems like the best way to avoid calling the event here. Otherwise, if the missing event calls were to be added; it would seem reasonable to add something to the event itself which denotes we don't want an updateState call to chain/trigger new BlockPhysics events. That route is a bit more fuzzy. Regardless of whether the event calls are added in, our server will resolve the reported behavior for ourselves. Mostly advocating for the broader community. |
| Comment by md_5 [ 11/Aug/18 ] |
|
x 6 overstates the impact that would have, and only affects servers doing that specific checking, and doing so inside an event is no doubt faster than firing 6 events anyway. Just jamming those extra event calls in creates a penalty for every server |
| Comment by chickeneer [ 11/Aug/18 ] |
|
I can agree to the fact that there are a ridiculous amount of physics events which occur. The one problem with your suggestion, is that a plugin then has to do 6 extra checks for EVERY BlockPhysics events (x6 instead of +6) - as the plugin cannot tell which physics event is the new one (unless it could be reliably cached from the location from where the plugin just allowed the break). I am at a loss of what 'should' happen. To keep consistent behavior - this case of events should be added without question. But in interest of reducing the number of event fires - it would be nice if the physics event here -https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/commits/f68afdb0caa9ed5b377f8fea3fbe0b3abebcd6fd could somehow be 'marked' that it will cause these new physics events to occur so a plugin could short it out.
The alternate route. Is to put it in the updateState method for BlockWallSign and BlockFloorSign. This is done in BlockPlant and BlockTallPlant for an unknown reason. This would seem like a more error prone option. |
| Comment by md_5 [ 11/Aug/18 ] |
|
I'm against this. The BlockPhysicsEvent is RIDICULOUSLY spammy as is. This would add SIX EXTRA physics events for EVERY block update.
Although this means you have to do some legwork yourself to catch the sign updates (check surrounding blocks), I think that is far better than firing an additional six events |