Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add-Mob-Spawner-support-to-AffectsSpawning #9578

Closed
wants to merge 5 commits into from
Closed

Add-Mob-Spawner-support-to-AffectsSpawning #9578

wants to merge 5 commits into from

Conversation

TreemanKing
Copy link
Contributor

Introduce API for Mob Spawner support for AffectsSpawning to allow different type of spawning types. The reason for this as it would allow for the ability to only prevent natural spawns and keep mob spawns from spawners.

Originally I was just going to create another get/set, however, the code for checkDespawn is quite hard to change without breaking a previous patch hence I opted for editing original API by adding an extra boolean to the get (public void setAffectsSpawning(boolean affects, boolean affectsMobSpawners))

I believe everything should work by common sense but unsure if I've missed or possibly stuffed something up as it's my first time meddling around with this.

@TreemanKing TreemanKing requested a review from a team as a code owner August 5, 2023 11:15
Copy link
Contributor

@Sytm Sytm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you made changes to the API of Paper but the patch for that is missing

};
+
+ public static final Predicate<Entity> PLAYER_AFFECTS_MOB_SPAWNER_SPAWNING = (entity) -> {
+ return !entity.isSpectator() && entity.isAlive() && entity instanceof Player player && player.affectsSpawning;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I see this correctly you want to do this?

Suggested change
+ return !entity.isSpectator() && entity.isAlive() && entity instanceof Player player && player.affectsSpawning;
+ return !entity.isSpectator() && entity.isAlive() && entity instanceof Player player && player.affectsMobSpawnersSpawning;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of? I want to make it only impact Natural Spawns and not spawns from Spawners themself.

Comment on lines 93 to 97
- public void setAffectsSpawning(boolean affects) {
+ public void setAffectsSpawning(boolean affects, boolean affectsMobSpawners) {
this.getHandle().affectsSpawning = affects;
+ this.getHandle().affectsMobSpawnersSpawning = affectsMobSpawners;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment this would be an API break.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess I would have to seperate this and make it seperately with a new set and get for this. I got stuck towards the section of src/main/java/net/minecraft/world/entity/Mob where you can't look at both. What I could do is seperate both so they are different but then again, this might also break API for those who use the method to get rid of all spawns which I want to prevent as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I have changed this, however, I am unable to figure out how to change the section listed (CheckDespawn) and SkeletonTrapGoals "canUse" section as it seems pre hardcoded in and I'm not too sure how to change this.


// Paper start - optimise checkDespawn
public final List<ServerPlayer> playersAffectingSpawning = new java.util.ArrayList<>();
+ public final List<ServerPlayer> playersAffectingNaturalSpawning = new java.util.ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment this is unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Will resolve this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be resolved.

@TreemanKing
Copy link
Contributor Author

TreemanKing commented Aug 5, 2023

It looks like you made changes to the API of Paper but the patch for that is missing

Oops I must've had it on the wrong directory, my bad.

EDIT: I think I've put it back but it attached to Whitelist Events API??? Not sure why it's done that.

@lynxplay
Copy link
Contributor

I am going to close this as I don't think a secondary check here is really the way to go.
As you already laid out, this logic is rough to edit. Breaking API for this is also 100% not worth it, given how niche this API is going to be.

An alternative approach that I could see working in the future would be an event called before the spawner searches for nearby players. To counterbalance the performance impact, this event might also allow for the definition of a backoff cooldown from even searching for players (while still running the rest of the tick logic).

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) still allows what this PR aimed to achieve, completely exclude players from affecting spawning on spawners.

I hope you can understand my points here, if you have further questions, we are always around on our discord :)

@lynxplay lynxplay closed this Aug 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

3 participants