Skip to content

Conversation

@GarrickWinter
Copy link
Contributor

Issue #41283 is currently disrupting the flow of the official 2D tutorial for users with external script editors.

This change is to update the tutorial to acknowledge this bug and provide newcomers with clearer guidelines on how to complete the tutorial despite the bug.

It also clarifies a potential point of confusion about how strictly function naming conventions must be adhered to.

@GarrickWinter
Copy link
Contributor Author

Thank you for the corrections AThousandShips!

@AThousandShips
Copy link
Member

These are unrelated changes, and should be kept in a separate PR

@GarrickWinter
Copy link
Contributor Author

Will do! I'm new to contributing to the project; thanks for outlining the norm.

@AThousandShips
Copy link
Member

At the same time please squash your commits into one, see here

@GarrickWinter
Copy link
Contributor Author

Okay, I believe that removes the unrelated changes from this PR. Going to go over and squash the commits in my other branch before making a PR for those.

@AThousandShips
Copy link
Member

Please go ahead and squash the commits in this one too, as it has 14 of them

@GarrickWinter
Copy link
Contributor Author

GarrickWinter commented Sep 15, 2023

This is embarrassing, but I don't know how to do that. I'm new to contributing to repositories other than my own on GitHub, so I'm still coming to grips with how the workflow works. I've tried squashing the 14 commits, but GitHub Desktop is giving me an error: Unable to squash. Squashing replays all commits up to the last one required for the squash. A merge commit cannot exist among those commits. I also can't squash subsets of the commits, even individual pairs.

@AThousandShips
Copy link
Member

AThousandShips commented Sep 15, 2023

You need to drop the merge commit you have, using "drop" as a command, and when updating your branch you need to use git push --force so you don't get the merge commits put in there

I'll take a look tomorrow about providing more detailed instructions by testing doing it myself

@paulloz
Copy link
Member

paulloz commented Sep 16, 2023

The note reads like writing an OnBodyEntered() method in your code and typing it as _on_body_entered in the connection window would work? Unless this is a new behaviour in recent 4.2 (I don't believe it is), that's not the case.

@AThousandShips
Copy link
Member

I don't have any problems using the command line interface, just using git rebase -i upstream/master, I'm not sure what you are using exactly, but if you use the command line it will be simple to fix it, I don't use the github desktop app myself

@GarrickWinter
Copy link
Contributor Author

The note reads like writing an OnBodyEntered() method in your code and typing it as _on_body_entered in the connection window would work? Unless this is a new behaviour in recent 4.2 (I don't believe it is), that's not the case.

You're right! I tested again this morning. I'm not sure where my original confusion about this came from, but I was convinced it was connecting despite the format of the function name not matching (I still changed it to match after coming to that conclusion, because I just really didn't like them not being exact matches).

I think the fact that the connection icon in the Node panel is bright green even if there's no connected function in the script might have contributed to the confusion. I've removed that part of my edit.

@GarrickWinter
Copy link
Contributor Author

I'm hoping to figure out how to squash these commits today. I've never used the command line with Git so it will be a learning process.

Issue #41283 is currently disrupting the flow of the official 2D tutorial for folks with external script editors.

This change is to update the tutorial to acknowledge this bug and provide newcomers with clearer guidelines on how to complete the tutorial despite the bug.

It also clarifies a potential point of confusion about how the editor communicates connection between signals and script functions.
@GarrickWinter GarrickWinter force-pushed the GarrickWinter-tutorial-text-patch branch from ac1e8a6 to bb82f03 Compare September 19, 2023 15:06
@GarrickWinter
Copy link
Contributor Author

There, I think that did it!

Copy link
Contributor

@skyace65 skyace65 left a comment

Choose a reason for hiding this comment

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

Looks good, just needs a few minor changes.

I agree with these suggested changes! Applying

Co-authored-by: Matthew <matthewehr@hotmail.com>
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
@skyace65 skyace65 merged commit 217bd3c into godotengine:master Nov 24, 2023
@skyace65
Copy link
Contributor

Thanks! Congrats on your first merged PR! I've done a squash and merge for this PR but as AThousandShips said previously, you should be keeping everything to 1 commit, I'm making an exception since this is your first merged PR.

In the future I'd recommend updating your branch as described in the doc page AThousandShips linked to so you don't have to do a rebase. If that's something you can't do with the GitHub desktop app I'd recommend trying a different workflow. Personally I use VisualStudio Code, but use whatever you find works best for you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:getting started Issues and PRs related to the Getting Started section of the documentation cherrypick:4.1 enhancement topic:dotnet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants