Incompatibility with standard fake player checks.

[Darkhax]

Hello,

Your mod replaces the vanilla player entity with your own. Doing this causes massive issues for mods that have checks for fake players. Here is an example of a typical fake player check.

    public static boolean isPlayerReal (Entity player) {
        return player != null && player.world != null && player.getClass() == EntityPlayerMP.class;
    }

The reason you check for that exact class is because FakePlayer extends EntityPlayerMP, and there are several mods with their own implementations of a fake player that do not use Forge's FakePlayer class.

This issue makes your mod incompatible with nearly all mods that depend on Bookshelf, and all of the other mods which do this type of check such as MLCore and it's dependents, MDECore and it's depdents, Spontaneous Collections, and many other mods.

[orbwoi]

Hello darkhax, sorry about the delayed response- been busy irl last few days.

Frankly, I agree with you about this situation, but let me explain a little bit about the constraints I am under and best in class object oriented programming practices.

Essentially, the universal remote mode is an extended player functionality mod. In other words, it wants to add a new core feature to the Minecraft engine (rather than build on existing forge features) and expose that though an easy to use remote. It does this by spoofing the player server and client position and look vector to the targeted mod in order to open the GUI from anywhere.

An older implementation did not expand the base player class held by Minecraft itself. However, many mods do range checks either in events or simply acquire the player object from somewhere other than the passed in arguments. In order to support multiple important mods, I decided to extend the base player object of Minecraft using reflection. This actually works quite well since I was able to find a way to distinguish which mod was asking for player position and lie about it only to the target mod, only about the target block(s), and only while the GUI was still open.

Unfortunately, as you say, this does trigger some fake player checks if written as you suggest above. I did encounter a check like that in IC2 but IC2 only uses that check only under certain circumstances and I was able to ensure it worked in the normal way (without a remote). I tested with every mod in ATM3 a while back and wasn't able to find any mods using that style of fake player check (other than IC2) so I was hoping it wasn't very common.

Now, in regards to the check itself. I understand the reasoning for it and the fact that many mods out there may use this style of check. However, as a senior software engine and mentor in my day job, I want to say that it isn't great code. The reason is that violates the open-closed principle of SOLID object oriented programming. We should be able to extend a class without breaking its compatibility with existing classes.

The goal is to check if this is a fake player so that we ensure the target gameplay element only works for the real player and not his devices. That is fine. However, the check doesn't actually do that. It just ensures the player's functionality isn't extend or modified. Those two checks are not the same thing. We should allow players to be extended but still block fake ones. An example of a different implementation which is not subject to this constraint would be something like the following:

public static boolean isPlayerReal (EntityPlayerMP player) {
    return FMLCommonHandler.instance().getMinecraftServerInstance().getPlayerList().getPlayers().contains(player);
}

Please don't take this suggestion as an insult or any kind of personal attack on you or the modded community. I love modded Minecraft and am still a bit new to writing forge mod (even though I'm a very experienced programmer). You personally have contributed quite a bit to the modded community so please let me know what you think.

Ultimately, I think it might be best for forge to provide an IsFakePlayer API which can be updated and standard across all mods. (Though really, the core feature needed by this mod- modifying player and server methods conditionally- shouldn't be implemented with hacky reflection at all. A clean and proper approach would add it directly to forge as a new set of hooks/apis. That solution would probably be able to avoid all these pitfalls as well.)

That said, I would love to be compatible with current mods like the ones you mention above without having them all update their code. Unfortunately I cannot think of a good way right now. I suppose I could add an exceptions list for mods in the config, but that isn't really a good long term solution. If you have any suggestions please let me know.

[Darkhax]

Thanks for the thoughtful response on this issue. I am aware that this code violates open-closed, and that's not something I am happy about. Unfortunately it has been a necessary evil when working with other mods.

The solution you have come up with is a seemingly acceptable one, however in the past there have been some mods that add their fake player to the player list. In one specific example they were using this to add messages to the player list gui from the server. I haven't seen any mods that do this in 1.12.2, but there is always the chance that those mods will be updated or new mods will take their place. Because of this, it's not a good long term solution on my end.

With all of this in mind, Forge has recently accepted this PR which does a check similarly to the way described in my initial comment. I think that this type of check being used in Forge is good enough for me to point to when it gets brought up.

As for how things could be improved on your end, have you considered opening some pull requests in Forge to help improve compatibility?

[orbwoi]

Thanks for the thoughtful reply. :)

Interesting- so they used the player list to display a message on the player list... that seems like a bit of a lazy approach to me. Has any mod you can think of ever BOTH done that AND had a fake player machine (esp. using the same player object)? It is possible to write something that breaks this- though it seem unlikely.

Also, I notice that the PR in forge is doing a blacklist check for FakePlayer not a whitelist for the default player class. They didn't break open/closed with that patch.

I haven't tried to make a pull request to forge yet. Before then, I need to more carefully consider exactly what I should ask them to put in forge and present it as an api that any modder can use. Also, it needs to not break if more than one person wants to use it. Are the pull reviewers for forge fairly friendly to newcomers?

It would be nice to finally add the real ability for the client to load chunks of other worlds (without the player being in that world) and get updates on them for looking through live portals to other dimensions (or remote GUI access like in my mod).

[orbwoi]

Closing issue since your change resolved the conflict.