Add SpawnerPlayerSearchEvent

[TreemanKing]

Hopefully second try is the charm. With the feedback of lynxplay from my previous pull, I created a new Event which would act before the Spawner searches for nearby spawners.

This would:
a) allow to remove specific players from specific spawners in regards to their activation
b) allow spawners to skip player searching for n ticks (as specified by the backoff cooldown)
c) completely exclude players from affecting spawning on spawners.

I was also going to add NotNulls to the event but I wasn't sure so your feedback would be highly appreciated!

[Warriorrrr]

PR looks good aside from a few small comments.

[Lulu13022002]

I forgot to say that in the review, but you need the notnull/nullable annotations in the API otherwise the test will fail

[TreemanKing]

I believe all of the above should be resolved. If someone could please check this, that would be great!

[lynxplay]

Generally really nice work 👍 a few annotations left but beyond that good and ready for testing/review.

[lynxplay]

Looks good to me, I'll test the PR later today when I make it back home.
The comments can be resolved by the merger or you in time, but should not prevent the PR from being merged 👍

[lynxplay]

After testing, seems to work well.
However, I am a bit bleh about the naming of the event after thinking about it.
It isn't really pre-search, the "heavy" searching logic is still executed.
The backoff ticks can prevent that, but the event is not really pre- anything.

[TreemanKing]

After testing, seems to work well.
However, I am a bit bleh about the naming of the event after thinking about it.
It isn't really pre-search, the "heavy" searching logic is still executed.
The backoff ticks can prevent that, but the event is not really pre- anything.

Yep, more I coded, the more I thought maybe it should just be called "SpawnerSearchEvent".
Edit: Updated all instances to "SpawnerSearchEvent"

[TreemanKing]

Should I move the event to Block instead of Entity as well?

[Lulu13022002]

It's a little bit tricky since the base spawner is used by the spawner block but also by the minecart spawner, i don't mind that whatever you chose but i think people are more used to see a spawner block

[Leguan16]

What is the general opinion on this one:
https://discord.com/channels/289587909051416579/555469074080202765/1148993218885718076
(adding Cancelable which defaults to DENY)

[lynxplay]

Again, i don't know if we have a precedent for this.
Issue isn't really setCancelled(true), issue is setCancelled(false).

Does it use DEFAULT or ALLOW ?
Its a bit meh

[TreemanKing]

Anything else I need to do or is this just awaiting reviews?

[lynxplay]

At least from my end, I just need to find time to give this a final review and testing 👍

[TreemanKing]

Having a second thought, maybe calling it SpawnerPlayerSearchEvent is more accurate with the event. Thoughts?

[lynxplay]

After some thought with MM about this, I think we have to rethink a bit how we want to handle the backoff ticks.

Mostly for the following scenario:
Plugin developer A sets the backoff ticks to 20.
Plugin developer A expects the spawner to start searching again in 20 ticks.
Server owner B set a tick delay for mob spawners in the paper world config to 20 ticks.
This means that every 20 ticks the spawner will attempt to search, only decrementing the backoff ticks every 20 ticks, leading to a total of 400 ticks before backoff ticks reaches 0.

Plugin developer A wants to avoid this, but is undable to do so because the tick delay is not exposed right now in the API so computing how long the backoff should be is not possible.

An alternative here would be to count down tickDelay and backoff ticks at the same time.
That way plugin developer A can set the backoff ticks to, lets say 20.
If the tick delay is e.g. 30, after 30 ticks the spawner would search again directly.

This way the plugin developer does not have to deal in spawner ticks but in world ticks, which should be a much more reliable/easy unit as it is not affected by the unaccessible tick delay.

[lynxplay]

The javadocs need updating, the event is called after players have been searched.

[Leguan16]

does the Spawner search Players? Might rename it to SpawnerSearchPlayerEvent?

[lynxplay]

This is not really needed, we are already checking the value to not be null in the setter.

[lynxplay]

Maybe my brain is fired here, but this would fail to reduce the backoff ticks if the tick delay is <= 0 after its decrementation.

If my server has a tick delay configure of 0, --backoffTicks > 0 is never evaluated.

[lynxplay]

Machine and I discussed this yesterday but wouldn't it make sense to "merge" backoffTicks with tickDelay ?

The tickDelay variable does pretty much the exact same.
We'd save ourself a field here by this.tickDelay = Math.max(this.tickDelay, event.getBackoffTicks()) instead.

Passing the current backoff ticks to the event seems, in retrospective, kind of useless ? given the event is only called when those reach 0, I don't think there is much value here.