Skip to content
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

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

Merged

Conversation

marcospb19
Copy link
Contributor

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 marcospb19 requested a review from a team as a code owner July 6, 2024 03:47
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Docs are pretty dang good now, assuming they are accurate.

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

modules/enet/doc_classes/ENetConnection.xml Outdated Show resolved Hide resolved
modules/enet/doc_classes/ENetConnection.xml Outdated Show resolved Hide resolved
@ze2j
Copy link
Contributor

ze2j commented Jul 7, 2024

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

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

@ze2j is right. Just putting these as additional suggestions so that they can be picked more easily.

modules/enet/doc_classes/ENetConnection.xml Outdated Show resolved Hide resolved
modules/enet/doc_classes/ENetConnection.xml Outdated Show resolved Hide resolved
@marcospb19
Copy link
Contributor Author

@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
Copy link
Contributor Author

marcospb19 commented Jul 8, 2024

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
Copy link
Contributor Author

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

@marcospb19 marcospb19 force-pushed the improve-enet-connection-docs branch from 04732c1 to 4a3fba1 Compare July 8, 2024 00:53
@marcospb19 marcospb19 requested a review from Mickeon July 8, 2024 00:54
@AThousandShips
Copy link
Member

AThousandShips commented Jul 8, 2024

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

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.

Thanks for adding these clarifications, looks good to me!

@marcospb19 marcospb19 force-pushed the improve-enet-connection-docs branch from 4a3fba1 to 4bf37c8 Compare July 11, 2024 01:41
@marcospb19
Copy link
Contributor Author

@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.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jul 17, 2024
@akien-mga akien-mga merged commit db95973 into godotengine:master Jul 17, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@Jimmio92
Copy link

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.

@Faless
Copy link
Collaborator

Faless commented Jul 26, 2024

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.

@Jimmio92
Copy link

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

@marcospb19 marcospb19 deleted the improve-enet-connection-docs branch July 27, 2024 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants