Skip to content

Update high level multiplayer manual for Godot 4. #7675

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 30, 2023
Merged

Update high level multiplayer manual for Godot 4. #7675

merged 1 commit into from
Jul 30, 2023

Conversation

Jordyfel
Copy link
Contributor

Updates the high level multiplayer manual for Godot 4.

I have used the class reference, blog posts and my experience as a user of the API to get the information. I expect to have missed some details as I don't know the internals of MultiplayerAPI.

@AThousandShips AThousandShips requested a review from Faless July 16, 2023 15:15
@AThousandShips AThousandShips added enhancement area:manual Issues and PRs related to the Manual/Tutorials section of the documentation labels Jul 16, 2023

To create that object, it first has to be initialized as a server or client.
Every node has a ``multiplayer`` property, which is a reference to a default ``MultiplayerAPI`` instance.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misleading, unless defined for the sub-tree it defaults to the default one, but this implies it's always a default one

get_tree().network_peer = peer
# By default, these expressions are equivalent.
multiplayer
get_tree().get_multiplayer()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, only if none has been configured, this is going to confuse people

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I put the "by default" for, but I can see it not being clear enough. I'm open to suggestions for better language since I'm not too sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this relevant? It's only true for a very specific case, so it's unhelpful information

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to specifically mention that Node.multiplayer is a reference to a "global" object, and not one unique per node. I've seen people ask why you cannot set this property even though you can get it. When learning the API I was myself confused about why one can just get multiplayer from every node and receive the same object.

Copy link
Member

@AThousandShips AThousandShips Jul 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then the correct code is get_tree().get_multiplayer(get_path())

Though this is already documented in Node

Copy link
Contributor

@FrederickKDP FrederickKDP Jul 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find get_tree().get_multiplayer() to be overly verbose. get_multiplayer() works on its own and makes more sense in my opinion. This is important for a tutorial.
Edit: Sorry, just read that multiplayer also works. It's also a good solution. Makes sense with the rest of Godot syntax of offering direct access and get & setters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are not equivalent, as is the entire point of this discussion here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are not equivalent, as is the entire point of this discussion here

I'm not saying that they are. I'm saying I'm not fond of using get_tree().get_multiplayer() for the reasons explained. If people really believe it's a must, I believe further explanation is necessary. Because otherwise, people will have the same doubts that we are having in this discussion.
Reasons to use get_tree().get_multiplayer() should be clear if used in my opinion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry your original comment made it seem that way based on the suggestion of replacing one with the other

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, no worries. It was confusing, the way I put it. My fault as well.
Always please feel free to argue my posts. I'm new here, so I appreciate a lot the feedback. Hopefully I may contribute to the discussion.

@Faless Faless requested a review from mhilbrunner July 16, 2023 23:59
@mhilbrunner
Copy link
Member

@Jordyfel Thanks for working on this, I've work in progress on expanding and reworking the networking sections, but as that turned out to take longer than anticipated I had planned to do a quicker update of the existing ones - so this is very welcome.

I'm currently building this page locally and will be reviewing it, thanks for contributing to Godot, especially to networking docs ❤️

Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I've finally found time to review this in depth. This is a great update to bring this page up to date for Godot 4.

I've found a few things, but these may as well be worth doing in followup PRs (by you, me or someone else):

  1. The warning about the Android INTERNET permission was removed in this PR. Was that intentional?
  2. Where it explains the signals, the signal names should probably be links to their class docs.
  3. I think the flow of the page could be improved (that has nothing to do with this PR though, it was already the case before): It starts with a lot of very high level general networking talk, then gets nitty gritty, and kinda goes back and forth and thats hard to follow. We should probably split the general networking overview, security considerations, comparing Godot's networking technologies etc. from the actual "this is how you use MultiplayerAPI".

It also needs advanced tutorials for the synchronizer tech etc, but that too is out of scope for this PR, just adding context.

All in all, I'll likely merge this sooner rather than later and iterate based on it, as I have further changes in my own fork and will work on the above (further PRs are of course welcome!)

Because as of right now, the current state of the page is badly outdated, so I'm in favor of improving it and then improving it further, rather than spending more time before merging. :)

@Jordyfel
Copy link
Contributor Author

I agree with everything you mentioned, this page probably has to be split/reorganized, though I'm not very confident in doing that since my networking knowledge is rather surface level. I just wanted to have all the basics in one place sooner rather than later because this API, more than most other, is a pain to figure out from the class reference as a new user (not so much the case for the scene replication nodes, which is why I didn't include them, though it would be better if they had tutorials as well).

I didn't notice the android permission warning, will put it back as it was.
I noticed a lot of class/method names could use links to the class reference, I just hadn't figured out how to do it when I was writing. Will try to get it done soon, or if you decide to merge before that, in a separate PR.

@FrederickKDP
Copy link
Contributor

I agree with everything you mentioned, this page probably has to be split/reorganized, though I'm not very confident in doing that since my networking knowledge is rather surface level. I just wanted to have all the basics in one place sooner rather than later because this API, more than most other, is a pain to figure out from the class reference as a new user (not so much the case for the scene replication nodes, which is why I didn't include them, though it would be better if they had tutorials as well).

I didn't notice the android permission warning, will put it back as it was. I noticed a lot of class/method names could use links to the class reference, I just hadn't figured out how to do it when I was writing. Will try to get it done soon, or if you decide to merge before that, in a separate PR.

Agreed. I was myself in this situation. It's very painful to figure it out as a novice.
I would argue that a point to have here is how in-depth should this page be. Is it for novices, or for pros. If is for newcomers, how pros will find clarification? If it tries to be for both, how to explain that without overly complicate for newbies?

@mhilbrunner mhilbrunner merged commit 0bf5bfe into godotengine:master Jul 30, 2023
@mhilbrunner
Copy link
Member

mhilbrunner commented Jul 30, 2023

Merged! Thanks and congrats @Jordyfel, this is amazing, doubly so for a first contribution :)

I would argue that a point to have here is how in-depth should this page be

Absolutely. We strive to keep things as welcoming as reasonably possible, but we absolutely do not try to teach people game development (or network engineering), that is way out of scope of the docs and our resources. The Godot docs are meant to be a technical manual for this specific game engine; creating in-depth networking/multiplayer tutorials is work for the greater community, we can only hope to manage to document the tech. (And even with that we really need help, as this PR shows! :))

@Jordyfel
Copy link
Contributor Author

Jordyfel commented Sep 5, 2023

Other issues I found which can seemingly also be closed by this: #5754, #7142. @mhilbrunner

Some guy on reddit complained that the issue he was tracking wasn't closed.

@mhilbrunner
Copy link
Member

Thanks for the ping, I closed those issues now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants