Fix ghost BlockStorage blocks due to iron golems, snow golems and wither spawning

[md5sha256]

Description

Fix ghost BlockStorage blocks due to players building/spawning iron golems, snow golems, and withers.\

Fixes 2425 (tagged below)

Proposed changes

Adds a new listener (CreatureBuildListener) which cancels CreatureSpawnEvents if one of the blocks used to build
either an iron golem, snow golem (EntityType#SNOWMAN), or wither is tracked by BlockStorage (BlockStorage#hasInfo == true)

Related Issues (if applicable)

Resolves #2425

Checklist

• I have fully tested the proposed changes and promise that they will not break everything into chaos.
• I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them.
• I have made sure that the proposed changes do not break compatibility across the supported Minecraft versions (1.16.* - 1.19.*).
• I followed the existing code standards and didn't mess up the formatting.
• I did my best to add documentation to any public classes or methods I added.
• I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values
• I added sufficient Unit Tests to cover my code.

[md5sha256]

Just a note, I haven't completed a full test with Slimefun/addons however I've manually tested the core logic of parsing the removed blocks (So implementation/utils/blockpattern/TShapedBlockPattern and implementation/utils/blockpattern/WitherBuildPattern) on a dev-plugin in my local workspace. I'll submit proper tests (and add unit tests) when I have a bit more time, however, I would like some sort of feedback.

A final note on possible improvements: this fix could be made significantly easier if a PR to spigot was made to add a new event which extends CreatureSpawnEvent that exposes the internal list of blocks which actually matched the build pattern.

[TheBusyBiscuit]

That's awesome!
Good and clean solution to a very awkward problem, thank you!

Does this also prevent the blocks from being removed or will it just cancel the mob spawning?
If not, then we may need to restore the blocks manually.

[md5sha256]

That's awesome! Good and clean solution to a very awkward problem, thank you!

Does this also prevent the blocks from being removed or will it just cancel the mob spawning? If not, then we may need to restore the blocks manually.

The current implementation just cancels the mob spawning as I'm not sure how to deal with whether or not the BlockStorage entry should be removed. The logic can definitely be changed to remove the BlockStorage entry however.

[TheBusyBiscuit]

I meaan the physical blocks, not the block storage data.
Like, will this leave the structure behind or will it create ghost blocks and air nonetheless.

[md5sha256]

I meaan the physical blocks, not the block storage data. Like, will this leave the structure behind or will it create ghost blocks and air nonetheless.

This will leave the structure behind, no physical blocks will be removed.
EDIT: The blocks are only removed by vanilla/craftbukkit if the CreatureSpawnEvent isn't canceled (I looked into the internals + verified by running a test in-game). Therefore, since the listener will cancel the event, the physical blocks won't be removed.

[TheBusyBiscuit]

I meaan the physical blocks, not the block storage data. Like, will this leave the structure behind or will it create ghost blocks and air nonetheless.

This will leave the structure behind, no physical blocks will be removed. EDIT: The blocks are only removed by vanilla/craftbukkit if the CreatureSpawnEvent isn't canceled (I looked into the internals + verified by running a test in-game). Therefore, since the listener will cancel the event, the blocks won't be removed.

Perfect! That's what I wanted to know.

[TheBusyBiscuit]

Also don't forget to mark params are nonnull and add Validate checks

[md5sha256]

Also don't forget to mark params are nonnull and add Validate checks

I added the class-level annotation ParamsAreNonnullByDefault, is that accepted or must I annotate the params individually?

[J3fftw1]

Marking this as stale.
Would be a nice fix to have in :D

[md5sha256]

Totally forgot about this, will push some changes in the next few days

[github-actions]

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: 26e448c9

https://preview-builds.walshy.dev/download/Slimefun/3796/26e448c9

Note: This is not a supported build and is only here for the purposes of testing.
Do not run this on a live server and do not report bugs anywhere but this PR!

[md5sha256]

I'm gonna leave a comment here before I push the commit which changes Validate for Preconditions. I can see Validate being used in other SF classes (ex: package io.github.thebusybiscuit.slimefun4.utils.biomes.BiomeMap), just making sure we do indeed want to use Preconditions. @JustAHuman-xD

[JustAHuman-xD]

I'm gonna leave a comment here before I push the commit which changes Validate for Preconditions. I can see Validate being used in other SF classes (ex: package io.github.thebusybiscuit.slimefun4.utils.biomes.BiomeMap), just making sure we do indeed want to use Preconditions. @JustAHuman-xD

We do, validate only runs on tests as we have found, there's an open issue about it iirc, new code uses preconditions until a better thing is created in dough

[md5sha256]

can we also unmark this as stale?

[variananora]

Just tested this on 1.20.1, there a few things that I found:

1. Snow golem will not spawn in any of the possible spawning rules (normal, upside down, or side)
2. Spawning iron golem in the default normal T position will not spawn the mob
3. Spawning iron golem side ways or upside down did spawn the mob and leaving ghost blocks behind.
4. Spawning wither using soul sand will not spawn the mob
5. Spawning wither using soul soil will spawn the mob and leaving ghost blocks behind.

Video: https://youtu.be/qaD1JPAZJqI

P.S. Here is a little addon to add snow block, soul sand, and soul soil for spawning those mob. (Use it on your own risk)

P.S.S. If you need me to test the build, feel free to ping me on Discord or Github and flag the PR "Needs Testing", I will look into it later!

[J3fftw1]

the edge cases you have found seem to be edge cases where spigot doesnt even check for.

[md5sha256]

the edge cases you have found seem to be edge cases where spigot doesnt even check for.

does paper check for these?

[md5sha256]

Just tested this on 1.20.1, there a few things that I found:

1. Snow golem will not spawn in any of the possible spawning rules (normal, upside down, or side)
2. Spawning iron golem in the default normal T position will not spawn the mob
3. Spawning iron golem side ways or upside down did spawn the mob and leaving ghost blocks behind.
4. Spawning wither using soul sand will not spawn the mob
5. Spawning wither using soul soil will spawn the mob and leaving ghost blocks behind.

Video: https://youtu.be/qaD1JPAZJqI

P.S. Here is a little addon to add snow block, soul sand, and soul soil for spawning those mob. (Use it on your own risk)

P.S.S. If you need me to test the build, feel free to ping me on Discord or Github and flag the PR "Needs Testing", I will look into it later!

interesting, i'm very surprised it is not working as expected, let me have another crack

[J3fftw1]

the edge cases you have found seem to be edge cases where spigot doesnt even check for.

does paper check for these?

dont know dont think they do var most likely tested on paper

[variananora]

dont know dont think they do var most likely tested on paper

Yeah I exclusively tested everything on paper these days

[md5sha256]

Ready for another round of testing. Fixes the issues with inverted orientations and the use of soul sand. Also added more unit tests for the new cases.
cc: @variananora for a re-test :)