Skip to content

Add Support for Scoreboard Teams on Folia#3296

Open
Jsinco wants to merge 11 commits intoCitizensDev:masterfrom
LumaLibre:master
Open

Add Support for Scoreboard Teams on Folia#3296
Jsinco wants to merge 11 commits intoCitizensDev:masterfrom
LumaLibre:master

Conversation

@Jsinco
Copy link
Contributor

@Jsinco Jsinco commented Feb 28, 2026

This PR adds support for the "scoreboardtrait" trait by introducing this library as a shaded dependency and sending all scoreboard and player-team related things with packets.

  • I want to note the fact that including this library may make Bukkit-based scoreboard teams obsolete because Citizens does not actually add players to teams and instead sends teams using packets. Citizens appears to only use Bukkit-based teams for registering and getting the client to recognize certain attributes of the team. This library does all of that without making any native calls so including a Bukkit implementation may no longer be required unless you think Citizens will need it.

I made my changes in the ScoreboardTrait class under the assumption that Citizens2 only runs on 1.21.1+ based servers (based on the NMS implementations available).

@CLAassistant
Copy link

CLAassistant commented Feb 28, 2026

CLA assistant check
All committers have signed the CLA.

Comment on lines +956 to +971
CitizensAPI.getScheduler().runEntityTask(npc.getEntity(), () -> {
boolean despawned = npc.despawn(DespawnReason.WORLD_UNLOAD);
if (event.isCancelled() || !despawned) {
for (ChunkCoord coord : Lists.newArrayList(toRespawn.keySet())) {
if (event.getWorld().getUID().equals(coord.worldUUID)) {
respawnAllFromCoord(coord, event);
}
}
event.setCancelled(true);
return;
}
event.setCancelled(true);
return;
}
if (npc.isSpawned()) {
toRespawn.put(new ChunkCoord(npc.getEntity().getLocation()), npc);
Messaging.debug("Despawned", npc, "due to world unload at", event.getWorld().getName());
}
if (npc.isSpawned()) {
toRespawn.put(new ChunkCoord(npc.getEntity().getLocation()), npc);
Messaging.debug("Despawned", npc, "due to world unload at", event.getWorld().getName());
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

The event cannot be canceled because you are delayed to the next tick, and therefore the event will already have been resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted changes because it's out of scope for this PR

Copy link
Contributor

@SNWCreations SNWCreations left a comment

Choose a reason for hiding this comment

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

Looks good. imo it is not necessary to introduce indent changes here.
And I think it is good for moving the codebase towards the lib even when on Spigot, since I know how the lib you used do its great work in my several projects. Also avoid unnecessary scoreboard data created by Citizens got saved by NMS when shutting down servers, provides a more clean experience.

@Jsinco Jsinco requested review from Euphillya and fullwall March 10, 2026 00:38
Comment on lines +86 to +94
public void sendToPlayer(Player player, SendMode mode) {
// TODO: Currently not implemented
/*if (mode == SendMode.ADD_OR_MODIFY) {
delegateDisplay.refresh();
delegateTeam.display(player, delegateDisplay);
} else {
delegateTeam.display(player, delegateTeam.defaultDisplay());
}*/
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a specific reason why this hasn't been implemented, or is it just an oversight?

Copy link
Member

@fullwall fullwall Mar 13, 2026

Choose a reason for hiding this comment

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

Agreed, TODOs should be finished

Copy link
Contributor Author

@Jsinco Jsinco Mar 20, 2026

Choose a reason for hiding this comment

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

Okay, after taking a look it just seems to be unnecessary. I had actually forgotten why I commented this out to begin with, but it was because the scoreboard library itself doesn't support not sending anything at all. There always has to be a TeamDisplay assigned to a ScoreboardTeam and all this method would do is just flip-flop between TeamDisplay objects. I commented it out and left the // TODO because I was going to look into it.

If you guys would prefer me to remove the TODO entirely, I can go ahead and do that.

@fullwall
Copy link
Member

Looks good, have you tested these changes actually load?

@fullwall
Copy link
Member

Can merge if someone can confirm that these are tested changes on a Folia server

@SNWCreations
Copy link
Contributor

Can merge if someone can confirm that these are tested changes on a Folia server

In the meantime, did these changes tested on regular Spigot/Paper server as well?

@Jsinco
Copy link
Contributor Author

Jsinco commented Mar 16, 2026

Will update this PR sometime tomorrow or the day after

@Jsinco
Copy link
Contributor Author

Jsinco commented Mar 20, 2026

I have only tested these changes on Folia and Canvas, not on Paper or Spigot. I imagine everything will work the same because I pretty much copied-pasted the original implementations into delegate classes, but it's possible I may have made a typo or something. I can test on Paper sometime tomorrow or this weekend but my schedule has been a bit random lately.

(I have been running this in production on my Canvas server.)

@Jsinco
Copy link
Contributor Author

Jsinco commented Mar 20, 2026

If there's anything else that needs to be addressed, feel free to let me know and I'll update this PR as soon as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants