Network Protocol Error Fix

[azure-server]

I think just removing from defaultDisplay would still create crashes, given displays can vary when using createDisplay. A workaround I found for this was iterating through online players and recursively removing from created displays from each team.

[vytskalt]

This can create some unexpected behavior now that I think about it.

For example we have players A, B, and C and two teams. Player A needs to see that player C is in the first team, while player B needs to see that player C is in the second team. This can be achieved right now by using multiple displays for each team. In this case we have the same player inside two different team displays, however it is technically safe because the displays are set up such that no single viewer can see a single player inside two different teams. So if we were to apply this fix, adding player C to the second team would remove them from the first team, making player A no longer see player C in any team anymore.

Now I remember why I haven't gotten around to fixing this...

[azure-server]

Hmm, then just iterating over defaultDisplays would allow correct behaviour for multiple displays, but still prevent network prot error, no?

[azure-server]

If it is set up like as you said, it won’t even matter if we don’t get rid of them from created displays because that won’t produce the client crash.

[vytskalt]

The scenario I mentioned above is only safe if the code is implemented correctly. If it were to accidentally mix up the displays and make one player be in multiple teams for any viewer then the viewer would crash.

[vytskalt]

I think the only way to 'fix' this would be to keep track of what entries each player sees for each team and then filter out sending any remove entry packets if it would cause a crash.

[azure-server]

@Override
public boolean addEntry(@NotNull String entry) {
  boolean conflictResolved = false;

  for (Player viewer : players) {
    for (ScoreboardTeam otherTeam : team.teamManager().teams()) {
      TeamDisplay otherDisplay = ((ScoreboardTeamImpl) otherTeam).display(viewer);
      if (otherDisplay != this && otherDisplay.entries().contains(entry)) {
        otherDisplay.removeEntry(entry);
        conflictResolved = true;
      }
    }
  }

  if (!conflictResolved && players.isEmpty()) {
    for (ScoreboardTeam otherTeam : team.teamManager().teams()) {
      TeamDisplay defaultDisp = otherTeam.defaultDisplay();
      if (defaultDisp != this && defaultDisp.entries().contains(entry)) {
        defaultDisp.removeEntry(entry);
      }
    }
  }

  if (!entries.contains(entry)) {
    entries.add(entry);
    team.teamManager()
      .taskQueue()
      .add(new TeamManagerTask.AddEntries(this, Collections.singleton(entry)));
    return true;
  }

  return false;
}

Think this would fix it? (I'm assuming this hasn't been looked at yet)

[vytskalt]

Looks very cursed but yeah, that would work I think

[azure-server]

Haha yeah, I think it looks bad from the use of the boolean at the start. I'll just integrate that better and update my PR.

[azure-server]

Let me know if this looks alright, or if there's any other edge cases you may see.

[vytskalt]

Well there is still the case when the player is added to the TeamManager and when the visible TeamDisplay is changed for a specific player. I don't think any automatic fixing logic is appropriate here. Perhaps it should just ignore the call and log to the console explaining what happened and how the developer needs to fix it.