godot icon indicating copy to clipboard operation
godot copied to clipboard

Clarify that `ENetConnection`'s `create_host` and `service` must be called on client and server

Open marcospb19 opened this issue 1 year ago • 1 comments
trafficstars

Context: despite 3 years working coding servers and clients, it took me around 10 hours to connect two ENet peers, and I read docs + ENet website + searched in issues and forums (oof!).

So, in this PR I try to clarify some of things I wish were there.

(In a separate PR I plan on adding an example.)

marcospb19 avatar Jul 06 '24 03:07 marcospb19

I may be wrong as I am not an english native speaker, but I think stablish should be changed to establish in the notes.

ze2j avatar Jul 07 '24 06:07 ze2j

@Mickeon

Also remember to squash your commits, see the Pull Request Workflow.

Sure I can do it, but GitHub already enables maintainers to squash and merge, and in the process, even rewrite the commit messages if they want to.

I see some back and forth in this repo like "change commit message" or "rebase every commit", while I see the reasons behind it, is there any reason why maintainers don't prefer to click on "Squash and Merge" button? Wouldn't it save time for both the maintainers and contributors?

I also ask this because I believe that fixup commits can be useful for reviewers!


Anyway! I won't block the PR on this question :) , applying changes and squashing, I appreciate the review.

marcospb19 avatar Jul 08 '24 00:07 marcospb19

Another small argument: Git beginners can have a hard time with interactive rebasing, IMHO this adds an unnecessary barrier to contributing, considering there is a button that solves it (one or two more clicks).

marcospb19 avatar Jul 08 '24 00:07 marcospb19

Done, applied all changes, ty @ze2j too!!!

marcospb19 avatar Jul 08 '24 00:07 marcospb19

GitHub already enables maintainers to squash and merge

It does (though it's not enabled on this repo), but sadly it doesn't allow squashing and creating a dedicated merge commit, which we require for any PR to make the git history cleaner and more manageable, it also helps reverting changes, and helps keeping the history clean in the rare cases where we don't enforce single commits, having merge commits also helps with cherry picking I believe

And rebasing and squashing is a core part of the workflow in any repo and any project and is a good and important skill to know, so at least I feel that it's less of a barrier and more a part of the general process of learning how to work with the tools and workflows necessary for contributing, just as much as learning how to make commits themselves or create branches! And maintainers are ready to help if new contributors are having a hard time and ask for help to figure it out, along with the detailed documentation

AThousandShips avatar Jul 08 '24 09:07 AThousandShips

@mhilbrunner

GitHub already enables maintainers to squash and merge

It does (though it's not enabled on this repo), but sadly it doesn't allow squashing and creating a dedicated merge commit, which we require for any PR to make the git history cleaner and more manageable, it also helps reverting changes, and helps keeping the history clean in the rare cases where we don't enforce single commits, having merge commits also helps with cherry picking I believe

And rebasing and squashing is a core part of the workflow in any repo and any project and is a good and important skill to know, so at least I feel that it's less of a barrier and more a part of the general process of learning how to work with the tools and workflows necessary for contributing, just as much as learning how to make commits themselves or create branches! And maintainers are ready to help if new contributors are having a hard time and ask for help to figure it out, along with the detailed documentation

Oh, I didn't know merge commits helped with cherry-picking and reverting changes! That's great to know.

Thanks for clarifying :)


CI failed after I applied this suggestion, it seems unrelated tho.

marcospb19 avatar Jul 11 '24 01:07 marcospb19

Thanks!

akien-mga avatar Jul 17 '24 10:07 akien-mga

You don't call create_host on clients -- that's what you call to start serving a session. If the client is the server, they can't be a client to someone else, so maybe this isn't worded right?

You do need to create an ENetConnection on all sides, client and server, but you don't call create_host on clients, you call create_client.

If for some reason you needed to create a host on clients to get connections to work, that's a bug imo.

Jimmio92 avatar Jul 26 '24 10:07 Jimmio92

You do need to create an ENetConnection on all sides, client and server, but you don't call create_host on clients, you call create_client.

@Jimmio92 You are confusing ENetConnection with ENetMultiplayerPeer. The first one, does indeed need either create_host or create_host_bound, and doesn't have a create_client method since it's almost a one-to-one mapping to official ENet API.

Faless avatar Jul 26 '24 11:07 Faless

Ahh, I now see I absolutely did confuse the two. Sorry about that!

Jimmio92 avatar Jul 26 '24 11:07 Jimmio92