add 'is_offline_peer' function to MultiplayerAPI

[ana-rchy]

addresses #75049

to-do: add documentation for function

might be a good idea to remove/deprecate has_multiplayer_peer as it will only ever return true

[dsnopek]

Thanks!

We do need some way to tell if we're really offline, since we'll now always have a multiplayer_peer set. However, I'm not sure a virtual method is the best way. We'll have exactly one class where this returns true, and we really don't want any others to implement the method.

Maybe we could change the behavior of has_multiplayer_peer() to check if it's the OfflineMultiplayerPeer or not? Something like return Object::cast_to<OfflineMultiplayerPeer>(*mutiplayer_peer) == nullptr.

From GDScript, maybe we could recommend if multiplayer.multiplayer_peer is OfflineMultiplayerPeer rather than having a new method for it?

[ana-rchy]

didnt realize OfflineMultiplayerPeer is exposed, but still feel there should be an “official” way to check for if the game is connected to any server
thats why i suggested removing has_multiplayer_peer so its replaced by is_offline_peer
i think other peer types should implement it so it returns false and you have a way to check if youre connected to any server

[dsnopek]

didnt realize OfflineMultiplayerPeer is exposed, but still feel there should be an “official” way to check for if the game is connected to any server

I agree! I'm just not sure a new virtual method is the right way to do it. I'd love to hear @Faless' and other members of the network team's opinions, though!

i think other peer types should implement it so it returns false and you have a way to check if youre connected to any server

We already have MultiplayerPeer.get_connection_status() to determine if a peer is connected or not. I'm not sure what extra value a dedicated is_offline_peer() method would add?

[ana-rchy]

get_connection_status cant return CONNECTION_DISCONNECTED as 4.0 introduced OfflineMultiplayerPeer instead of having a null value when disconnected; so now the ternary expression with the is_valid() condition always returns true given nothing goes wrong in the engine itself
i initially did try to change the return value to CONNECTION_DISCONNECTED but it threw alot of errors so i instead opted to make a new virtual function

even if you were to aim to change get_connection_status, youd end up needing to bind it to a different virtual method, so might aswell just have a new method to replace has_multiplayer_peer?

if we can figure out what causes the errors when making OfflineMultiplayerPeer return CONNECTION_DISCONNECTED in get_connection_status, itd likely be a much cleaner fix

[dsnopek]

get_connection_status cant return CONNECTION_DISCONNECTED as 4.0 introduced OfflineMultiplayerPeer instead of having a null value when disconnected; so now the ternary expression with the is_valid() condition always returns true given nothing goes wrong in the engine itself
i initially did try to change the return value to CONNECTION_DISCONNECTED but it threw alot of errors so i instead opted to make a new virtual function

Oh, no, I'm not suggesting using get_connection_status() to detect OfflineMultiplayerPeer. I was responding specifically to this comment, saying that is_offline_peer() on other children of the MultiplayerPeer class should return true/false depending if they were connected to the server:

i think other peer types should implement it so it returns false and you have a way to check if youre connected to any server

I was trying to say that I don't think this behavior adds value, because we already have get_connection_status() on those other peers. It would mean that for all classes other than OfflineMultiplayerPeer, the implementation would basically be:

bool is_offline_peer() const { return get_connection_status() != CONNECTION_CONNECTED; }

The approach to this overall problem that I personally like the best (again, we need some input from the networking team - this is just my opinion :-)) is changing MultiplayerAPI::has_multiplayer_peer() to return false if the peer is OfflineMultiplayerPeer. The name of the method isn't ideal, so we could also add a better named method (for example, MultiplayerAPI::is_offline()) and deprecate MultiplayerAPI::has_multiplayer_peer(). But, in general, I feel like giving this responsibility to MultiplayerAPI makes more sense than adding a new virtual method to MultiplayerPeer.

Perhaps, I'll make an alternative PR with this approach so they can both be evaluated...

[dsnopek]

I made the alternative PR described above to facilitate discussion: #75185

[Faless]

This can be checked via: ```gdscript multiplayer.multiplayer_peer is OfflineMultiplayerPeer ``` There is no need to add a method to the interface.

…

On Sun, Mar 19, 2023, 19:52 David Snopek ***@***.***> wrote: @dsnopek <https://github.com/dsnopek> requested review from @godotengine/network on: #75121 <#75121> add 'is_offline_peer' function to MultiplayerAPI. — Reply to this email directly, view it on GitHub <#75121 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM4C3R22KUJD5MAD2BBHGDW47A7HANCNFSM6AAAAAAWAMRSRM> . You are receiving this because your review was requested.Message ID: ***@***.***>

[PierceLBrooks]

I think the idea of whether or not a peer is "remote" would be better as far as eliminating the epistemological collisions arising from the notion of an "offline" peer.