> Hiding and then showing a player (who is not currently hidden by other plugins) should still do the same thing after this patch as it did before this patch.

You are correct on this: Invisibility-bugs shouldn't matter if the affected player is meant to be invisible anyways.

> [..]

You are correct that there are various potential issues, before and after this change. So far however I have considered most of those issues (like no fake join message or the vanish plugin reporting that the player is still vanished) to be less of importance compared to players fighting against an invisible player. So forcefully making players visible is indeed kind of a fallback here.

> A minigame plugin could instead detect invisible players by calling canSee between each pair of players, and prevent a player from entering the minigame if they're invisible to anyone in the minigame.

In the common case of vanishing-plugins, preventing vanished players from joining the minigame in the first place seems indeed to be a better solution.

However, I don't think that canSee can be used to properly distinguish this 'is-vanished' condition from a case like your NonEuclid plugin, which uses player hiding for temporary/local visual effects that get automatically and immediately reset again once the player gets teleported to a different (unaffected) location (in which case I would not want to prevent the player from joining the minigame).

> And/or it could check the "vanished" metadata value of each player, and encourage admin invis plugins to use that metadata value like SuperVanish does.

This looks like to be a proper solution to the compatibility issues with vanishing-plugins. Unfortunely, not even big plugins like Essentials use that/any metadata value for that purpose so far. So I will have to try to get something like this added to Essentials, and all the Essentials-like plugins that I come across that offer a similar feature.

However, for the time being and in case that there are other types of not yet accounted for plugins that as well don't automatically and in a timely manner reset their hiding state I would (as fallback) still need to forcefully make players visible again.

> Yes, if a plugin disables while a player is invisible, then they'll stay invisible. This was still an issue before this patch. (Personally I think it would be a good improvement to Spigot to detect when a plugin disables and leaves players invisible to each other, maybe log a warning about the offending plugin, and undo the invisibility. Before this patch, it wouldn't have been cleanly possible for Spigot to identify the connection between a plugin being disabled and a player's invsibility, but now the offending plugin can be identified if it uses the new API.)

I agree. And just for the record, overall I agree with this change as well. I just think that there can indeed be situations (like this minigame usecase) in which it might be useful to be able to force player visibility from within a plugin. And right now there doesn't seem to be a proper way to do that.

I saw that there was some discussion on how to react to a plugin unloading/disabling in the PR: Wouldn't it be possible to do this similarily to how a plugin's running scheduler tasks get automatically cancelled during plugin disable (see https://hub.spigotmc.org/stash/projects/SPIGOT/repos/bukkit/browse/src/main/java/org/bukkit/plugin/SimplePluginManager.java#427)? Basically inside Bukkit when a plugin got disabled, this would have to iterate all online players and remove the plugin for the 'hidden' sets (for example by calling showPlayer for all pairs of players).

>Minecraft has been affected by various invisibility issues in the past ... Hiding and then showing players via these methods was a workaround to fix this in the past. I don't know if this is still an issue today, but I have kept this workaround available as option so far just in case.

Hiding and then showing a player (who is not currently hidden by other plugins) should still do the same thing after this patch as it did before this patch.

>Still in the context of minigame plugins: Assuming an admin has marked himself as invisible, but forgets about it and then enters to participate in a minigame, the minigame plugin might want to forcefully make the player visible again in that case.

Before and after this patch, I don't think Spigot ever offered a good solution to this. If someone uses an admin invis plugin to turn on invisibility against all other players, enters into a minigame where the minigame plugin forces visibility against all other players, then there's a number of bad consequences:

• the admin will have invisibility against any new players that enter the minigame, unless the minigame plugin paranoidly re-forces visibility between new players that connect during the minigame and all of the current players.
• The admin's invis plugin will incorrectly tell the admin that they're still invisible (unless it paranoidly periodically calls canSee against players that it previously tried to hide itself from).
• if the admin wants to re-enable invisibility, their admin invis plugin may think they're still invisible and not do anything if they try to toggle it on.
• I remember that at least one admin invis plugin I found while researching about this issue would paranoidly periodically re-apply invisibility of people that were using it, so even if the minigame forced visibility, this kind of admin invis plugin would fight back after a short time.
• Some admin invis plugins take extra steps to hide invisible players from the player list, and show a fake join message when the player leaves invisibility. Force-showing a player won't notify the plugin (unless it is re-checking canSee to see if its changes were clobbered) to announce the invisible player as joining the game.

(But yes, if you iterate over the plugins and call showPlayer between each pair of players, then it will be equivalent to the pre-patch consequences of calling showPlayer between each pair of players.)

A minigame plugin could instead detect invisible players by calling canSee between each pair of players, and prevent a player from entering the minigame if they're invisible to anyone in the minigame. (And/or it could check the "vanished" metadata value of each player, and encourage admin invis plugins to use that metadata value like SuperVanish does.) The minigame plugin could then choose to go above-and-beyond by integrating with some of the more popular admin invis plugins so the minigame plugin could automatically tell the admin invis plugin to reveal the player when they enter a minigame.

>What about disabled plugins: Since the currently implementation uses weak references instead of reacting to plugin disables: Is it possible for disabled plugins to still force players to stay invisible (ex. if a reference to the plugin still exists somewhere)?

Yes, if a plugin disables while a player is invisible, then they'll stay invisible. This was still an issue before this patch. (Personally I think it would be a good improvement to Spigot to detect when a plugin disables and leaves players invisible to each other, maybe log a warning about the offending plugin, and undo the invisibility. Before this patch, it wouldn't have been cleanly possible for Spigot to identify the connection between a plugin being disabled and a player's invsibility, but now the offending plugin can be identified if it uses the new API.)

During the updating of an old plugin for 1.13, I stumbled across this change and have two usecases in which a plugin would have to forcefully make certain players visible to each other (which were broken by this change):

• Minecraft has been affected by various invisibility issues in the past (see https://bugs.mojang.com/browse/MC-65040 and all related issues). For example, if a minigame plugin teleports a bunch of players to some location at the same time, those players might end up not seeing each other. Hiding and then showing players via these methods was a workaround to fix this in the past. I don't know if this is still an issue today, but I have kept this workaround available as option so far just in case.
• Still in the context of minigame plugins: Assuming an admin has marked himself as invisible, but forgets about it and then enters to participate in a minigame, the minigame plugin might want to forcefully make the player visible again in that case.

If I understand it correctly, the suggested solution for forcefully making a player visibile would now be to iterate over all plugins (returned by the PluginManager) and call showPlayer with them as argument? What about disabled plugins: Since the currently implementation uses weak references instead of reacting to plugin disables: Is it possible for disabled plugins to still force players to stay invisible (ex. if a reference to the plugin still exists somewhere)? How would one go about forcefully overwritting their behavior in this case?

Thanks for your contribution, the PRs have been pulled

Note that multiple WeakReferences to the same object aren't .equals() to each other, which makes it not play nice with Sets, unless you keep a canonical weakreference for every plugin around somewhere, so I've had to add a private static WeakHashMap<Plugin, WeakReference<Plugin>> field that showPlayer/hidePlayer make use of.

The old methods could be equivalent to passing null as the plugin parameter. (So multiple plugins using the old methods will continue the same overriding-each-other behavior between each other, but won't collide with any plugins using the new API.)

I've submitted the pull requests: https://hub.spigotmc.org/stash/projects/SPIGOT/repos/bukkit/pull-requests/307/overview, https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/pull-requests/409/overview

Seems like a reasonable proposal.

Type definitely does have to be plugin. It probably doesn’t need to be removed onDisable, but in that case it does have to be a weak reference.

Curious if there are any other API methods where this logic could be applied to.

Also needs to preserve existing behaviour 

The key parameter could be of only type Plugin instead (and just named "plugin"); that would make its meaning a lot more obvious to users and make it harder to accidentally misuse. I originally avoided suggesting that because it would also imply that Spigot will automatically remove the hide assertion when the plugin is disabled. (Since that seems to be the behavior everywhere else that Plugin is accepted as a parameter.) That probably would be a good behavior; I just shyed away from suggesting a slightly bigger change.

The author of the Dynmap plugin came up with a good solution for this:
https://github.com/webbukkit/dynmap-api/blob/master/src/main/java/org/dynmap/DynmapAPI.java#L59