DoubleDeez / MDFramework

A multiplayer C# game framework for Godot 3.4 Mono.
https://discord.gg/UH49eHK
MIT License
77 stars 13 forks source link

Feature: Multiple ideas for improvements #39

Closed Meister1593 closed 4 years ago

Meister1593 commented 4 years ago

I've played quite a bit with this framework and would like to share some thoughts and ideas which would be really helpful in that framework.

  1. IsConnected boolean in GameSession, or GameInstance (just somewhere where you can get it form anywhere like these) This was a bit of a struggle since i didn't know that GameSession.IsSessionStarted means that he successfully connected. This should be either renamed, or left like that but this bool doesnt mean that player is synched. Would be a great idea to combine GameSession.IsSessionStarted and some boolean about synch to know if we were connected and synched first time. Would be also nice to make event after started and synched session.

  2. PlayerCount This is needed sometimes too. What i've done, is inherited from MDGameInstance and exposed Player.Count with a function. Really simple, but very helpful

  3. Getting player info before it leaves https://github.com/DoubleDeez/MDFramework/blob/3a72a950591c892350362a6844992f544a00aa68/MDNetworking/MDGameSession.cs#L291-L295 This mostly needed for getting information about player before it disconnecting, for example to print it's name from peerid, Currently, first the player leaves (thus, info was deleted) and then all notified about his leaving. I know that in BasicNetworkLobby there is a info about that override, but i think that would be nice to have inside framework. https://github.com/DoubleDeez/MDFramework/blob/3a72a950591c892350362a6844992f544a00aa68/Examples/BasicNetworkLobby/BasicNetworkLobby.cs#L118-L123 What i've done, is inherited from MDGameSession and made new event like that:

    public class MyMDGameSession : MDGameSession
    {
        public event PlayerEventHandler OnPreparePlayerInfoForRemoval = delegate { };
    
        protected override void PreparePlayerInfoForRemoval(MDPlayerInfo PlayerInfo)
        {
            OnPreparePlayerInfoForRemoval(PlayerInfo.PeerId);
        }
    }

    (might be a good idea to send not just peerid, but a full info there, it was just for example) And then used this MyMDGameSession with cast from getting this.GetGameSession()

  4. Ability to disable ServerRelay for both client and server https://github.com/DoubleDeez/MDFramework/blob/3a72a950591c892350362a6844992f544a00aa68/MDNetworking/MDGameSession.cs#L86-L88 https://github.com/DoubleDeez/MDFramework/blob/3a72a950591c892350362a6844992f544a00aa68/MDNetworking/MDGameSession.cs#L113-L115 This can be changed easily by placing this peer.ServerRelay = param;, on 87 and 114 line above, where param is bool passed into a function. This allows for full server authoritative setups, where you don't automatically relay to clients using server, but rather authorize it and process in between.

Beider commented 4 years ago
  1. Did you check the MDStatics class? It has a few methods such as MDStatics.IsNetworkActive(), MDStatics.IsServer() and MDStatics.IsClient(). Documenation is a bit lacking at the moment. I do plan on making some video tutorials and writing some guides in the wiki once the framework is in a bit more of a complete state.

  2. I agree with this one, in theory you could use MDGameSession.GetAllPlayerInfos() and do a count on that, but exposing player count directly would be better.

  3. I agree with this as well, I think we should just swap the order of the MDPlayerInfo being removed and the event being sent out. It would make a lot more sense to send out the event while the player info still exists.

  4. Probably also worth adding.

I would wait to see what @DoubleDeez says about this, but I would think you could just make those three changes on your fork and just create a pull request.

DoubleDeez commented 4 years ago
  1. The helpers Beider mentioned are probably what you want. As for the synch'd event, there's OnSynchCompleteEvent on MDGameSynchronize. We do need to improve our documentation so these are easier to find. https://github.com/DoubleDeez/MDFramework/blob/3a72a950591c892350362a6844992f544a00aa68/MDNetworking/MDGameSynchronizer.cs#L30-L31
  2. Yup, I agree we should add this.
  3. I can't think of any reason to remove the object before sending the event so yeah I think we should just swap the order
    private void OnPlayerLeft_Internal(int PeerId) 
    {
     OnPlayerLeftEvent(PeerId);
     RemovePlayerObject(PeerId);
    } 
  4. That's a good idea. There's probably other settings we should be exposing too, but instead of having to add a bunch of parameters I wonder if we should have a virtual void ConfigurePeer(peer); function or something along those lines.

and yeah, if you can, please create a pull request with these changes, otherwise we'll get to it when we have the time.

Meister1593 commented 4 years ago

Ok i will make pull request, but:

  1. The helpers Beider mentioned are probably what you want. As for the synch'd event, there's OnSynchCompleteEvent on MDGameSynchronize. We do need to improve our documentation so these are easier to find. https://github.com/DoubleDeez/MDFramework/blob/3a72a950591c892350362a6844992f544a00aa68/MDNetworking/MDGameSynchronizer.cs#L30-L31

This does work, but this triggers every time synch is happening, for every player every time someone joins.

  1. Did you check the MDStatics class? It has a few methods such as MDStatics.IsNetworkActive(), MDStatics.IsServer() and MDStatics.IsClient(). Documenation is a bit lacking at the moment. I do plan on making some video tutorials and writing some guides in the wiki once the framework is in a bit more of a complete state.

MDStatics.IsNetworkActive() seems only count that we're connected but isn't synched yet. For event this needs custom logic, i've used this to actually know if i'm synched or not and exposed this to my GameSynchronizer. https://github.com/DoubleDeez/MDFramework/blob/3a72a950591c892350362a6844992f544a00aa68/MDNetworking/MDGameSynchronizer.cs#L60

I'm planning to expose this var and make event that happends inside OnSynchCompleteEvent. Not sure if that's right to do nested events.

Beider commented 4 years ago

Alright so if I get you right you want an event that tells you the first time a player has finished synching? Should this be on master, the joining player or all players?

I would suggest adding a completely new event called something like OnPlayerSynchComplete(int PeerId)

The best place to add this is probably to this method in the game synchronizer,

[Puppet]
protected void UpdateSynchStatus(int PeerId, float SynchStatus)
{
    OnPlayerSynchStatusUpdateEvent(PeerId, SynchStatus);
}

Just change OnPlayerSynchStatusUpdateEvent(Multiplayer.GetRpcSenderId(), 1f); in ClientSynchDone() so it also calls UpdateSynchStatus(...)

Then just check if the synch status is 1f, which means it is done, in which case you can send the event. You would probably have to introduce another list like the ClientSynchList, something like InitialSychDoneList to track so it only happens the first time. And remove them from the synch list if they leave.

Other alternative would be simply to subscribe to the OnPlayerSynchStatusUpdateEvent somewhere in your own code, then check when it is 1f and send your event there. Then you would also have to track which peers this event has been sent for already but would be the best solution if you don't want to change the framework.

Meister1593 commented 4 years ago

Alright so if I get you right you want an event that tells you the first time a player has finished synching? Should this be on master, the joining player or all players?

This event should be invoked on player who joins to server, only one time when he finished connection and finished synching.

Beider commented 4 years ago

The easiest way to do this without changing the framework would be for you to subscribe to the OnPlayerSynchStatusUpdateEvent

Then just do something like this

protected bool AlreadySynched = false;
    protected void SynchStatusUpdate(int PeerId, float ProgressPercentage)
    {
        if (AlreadySynched)
        {
            return;
        }

        // Float compares are annoying
        if (PeerId == MDStatics.GetPeerId() && ProgressPercentage >= 0.99f)
        {
            // Do your stuff here
            AlreadySynched = true;
        }
    }

Edit: I added this example to the wiki page for the Game Synchronizer.

Meister1593 commented 4 years ago

The easiest way to do this without changing the framework would be for you to subscribe to the OnPlayerSynchStatusUpdateEvent

Then just do something like this

protected bool AlreadySynched = false;
    protected void SynchStatusUpdate(int PeerId, float ProgressPercentage)
    {
        if (AlreadySynched)
        {
            return;
        }

        // Float compares are annoying
        if (PeerId == MDStatics.GetPeerId() && ProgressPercentage >= 0.99f)
        {
            // Do your stuff here
            AlreadySynched = true;
        }
    }

This is actually nice, i might just use that for my event. Is that good idea to make NodeSynchCompleted exposed in case user needs to know or it is not?

Beider commented 4 years ago

I don't think there is any point in exposing NodeSynchCompleted in the framework itself as the event mentioned above pretty much gives you that information. Of course if you really want it I would suggest just overriding the MDGameSynchronizer and exposing it.