paper

[rustic folio]

NamiUni

[PaperMC/Paper] New comment on pull request: #13030 Add LootTableRollEvent

Just like the original pull request, I haphazardly placed this in the server package, but I feel it might be better to create a new package for it. I just can't think of a good package name 🤔

[rustic folio]

Tmpod

[PaperMC/Paper] Pull request opened: #13080 Add a new event for when an entity's scoreboard tags are changed: `ScoreboardTagsChangeEvent`

This adds a new event, ScoreboardTagsChangeEvent, which is called when an entity gets their scoreboard tags changed, through plugins calling Entity#addScoreboardTag, /tag, /data, etc.
It is fired before the change happens and can be cancelled. It contains the affected entity, which change it is happening (add, remove or replace), and which tags are part of the change.

I was looking to implement this event, because I'm currently emulating it, on a plugin I'm developing, by doing some complex and brittle handling of commands. However, it can't handle plugins calling addScoreboardTag/addTag and it isn't even complete for vanilla methods (e.g. has issues with execute context changes).
So I thought of adding this small event, it shouldn't impact performance that much. Even though I wasn't originally thinking of doing it, I made it cancellable, because why not?
I do have one concern with the current implementation, which is that it always creates a new list, even for single tag c...

[rustic folio]

Tmpod

[PaperMC/Paper] Pull request marked as draft: #13080 Add a new event for when an entity's scoreboard tags are changed: `ScoreboardTagsChangeEvent`

electronicboy

[PaperMC/Paper] New comment on pull request: #13080 Add a new event for when an entity's scoreboard tags are changed: `ScoreboardTagsChangeEvent`

We generally dislike low level zero-contextual events to be cancellable, especially in cases where plugin calls will be involved; These events are okay for notification purposes (assuming they're not too heavy), but, generally, "plugins actions are exposed to everything else" is a pretty niche area due to just not breaking stuff blindly

[rustic folio]

Tmpod

[PaperMC/Paper] New comment on pull request: #13080 Add a new event for when an entity's scoreboard tags are changed: `ScoreboardTagsChangeEvent`

In this case, addTag/removeTag (and their Bukkit Entity counterparts) can already fail and not do anything (and thus return a boolean), so it doesn't seem completely out of place to have them be cancellable. This does implement cancellation for NBT changes (e.g. through /data), which I can see being problematic. In any case, I am okay with removing cancellation altogether, if you prefer it so.

As for "plugins actions are exposed to everything else", I can see the reservations, though tags can be manipulated by means other than plugins.

electronicboy

[PaperMC/Paper] New comment on pull request: #13080 Add a new event for when an entity's scoreboard tags are changed: `ScoreboardTagsChangeEvent`

The only reason an add would currently fail is either that the tag already existed or you hit the size limit;

The concern here is that plugins have 0 real context of why a tag was being mutated, exposing such events for the purposes of notification to allow plugins to update stuff like a cache sounds fair, allowing plugins to blindly prevent a tag from being added to an entity is a bit more iffy

Tmpod

[PaperMC/Paper] New comment on pull request: #13080 Add a new event for when an entity's scoreboard tags are changed: `ScoreboardTagsChangeEvent`

Alright, I see, that is indeed iffy. I will completely remove cancellation then.

Tmpod

[PaperMC/Paper] New comment on pull request: #13080 Add a new event for when an entity's scoreboard tags are changed: `ScoreboardTagsChangeEvent`

Regarding naming and/or structure, is there any glaring issue? Apart from it being a feature patch.

Tmpod

[PaperMC/Paper] New comment on pull request: #13080 Add a new event for when an entity's scoreboard tags are changed: `ScoreboardTagsChangeEvent`

There it goes, amended the commits. Do you prefer squashing them both into one?

electronicboy

[PaperMC/Paper] New comment on pull request: #13080 Add a new event for when an entity's scoreboard tags are changed: `ScoreboardTagsChangeEvent`

The merger can deal with that aspect;

Tags should probably not be mutable inside of the event, and the event generally shouldn't fire for loading data

[rustic folio]

Tmpod

[PaperMC/Paper] New comment on pull request: #13080 Add a new event for when an entity's scoreboard tags are changed: `ScoreboardTagsChangeEvent`

Tags should probably not be mutable inside of the event

Good point. Would Collections.unmodifiableList be sufficient here, or do we want to go for a copy (with List.copyOf)?

the event generally shouldn't fire for loading data

Mmm, I see the concern. If we don't include that, modifying through NBT won't be covered. We can either accept that (I'd rather not, but ultimately it is okay if you prefer that) or opt to fire the event in the /data implementation, which would cover pretty much everything, I think.

electronicboy

[PaperMC/Paper] New comment on pull request: #13080 Add a new event for when an entity's scoreboard tags are changed: `ScoreboardTagsChangeEvent`

unmodifiableList would be fine;

firing inside of data is probably going to be the best option here

Tmpod

[PaperMC/Paper] New comment on pull request: #13080 Add a new event for when an entity's scoreboard tags are changed: `ScoreboardTagsChangeEvent`

There we go. Just missing documentation on the public stuff.

[rustic folio]

Tmpod

[PaperMC/Paper] Pull request marked for review: #13080 Add a new event for when an entity's scoreboard tags are changed: `ScoreboardTagsChangeEvent`