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 SceneTree.current_scene functionality #80264

Merged
merged 1 commit into from
Aug 7, 2023
Merged

Clarify SceneTree.current_scene functionality #80264

merged 1 commit into from
Aug 7, 2023

Conversation

AttackButton
Copy link
Contributor

@AttackButton AttackButton commented Aug 4, 2023

Improving the current_scene description, which was always very succinct. Now it clearly expresses the possibility of quickly getting the top node of the currently running scene.

This change is compatible with 3.x. (Bugsquad edit, needs modifications for the names of the methods for changing scene)

@AttackButton AttackButton requested a review from a team as a code owner August 4, 2023 16:20
@YuriSizov
Copy link
Contributor

YuriSizov commented Aug 4, 2023

Thanks for opening a PR! Why does your edit basically repeat the same thing twice?

@AttackButton
Copy link
Contributor Author

I wanted to emphasize the fact of getting the top node quickly, something not as simple as it seems. Even using owner or get_parent (successively until null is returned). I will rewrite the note in another way.

@YuriSizov
Copy link
Contributor

Notes should be used for important but tangential mentions. Here you can just continue the paragraph with an extra tip.

@AttackButton
Copy link
Contributor Author

It's ok now.

@AttackButton AttackButton requested review from a team as code owners August 5, 2023 11:56
@AThousandShips AThousandShips removed request for a team August 5, 2023 11:57
@AThousandShips
Copy link
Member

You should rebase instead of merging into your branch, you will need to clean up your branch

@AttackButton AttackButton requested a review from a team as a code owner August 5, 2023 12:06
@AThousandShips
Copy link
Member

AThousandShips commented Aug 5, 2023

See here, do not merge like this

@AThousandShips AThousandShips removed the request for review from a team August 5, 2023 12:08
@AThousandShips
Copy link
Member

AThousandShips commented Aug 5, 2023

What you need to do is:

  • Run git rebase -i master (assuming you've updated master, otherwise if you've added the upstream remote you can use upstream/master)
  • replace "pick" for all the commits that aren't yours with "drop"
  • replace "pick" of all your own commits save for your first one with "fixup"
  • Run git push -f (this is important)

@AThousandShips AThousandShips changed the title Explained current scene functionality. Clarify SceneTree.current_scene functionality. Aug 5, 2023
@AThousandShips
Copy link
Member

Please squash your commits into a single commit as described above

@AttackButton
Copy link
Contributor Author

I fixed it. Need to squash, I think.

@AThousandShips
Copy link
Member

AThousandShips commented Aug 5, 2023

Are you using git push -f? Because you keep making merge commits for some reason

@AttackButton AttackButton requested review from a team as code owners August 5, 2023 21:01
@AttackButton
Copy link
Contributor Author

I believe it is fixed now. Thanks.

@AThousandShips AThousandShips removed request for a team August 6, 2023 06:16
@YuriSizov YuriSizov changed the title Clarify SceneTree.current_scene functionality. Clarify SceneTree.current_scene functionality Aug 7, 2023
@YuriSizov
Copy link
Contributor

I believe it is fixed now. Thanks.

Unfortunately, not exactly. You had your branch name as your commit message. I fixed it and pushed on your behalf, so it should be good now.

@akien-mga akien-mga merged commit 01758ea into godotengine:master Aug 7, 2023
15 checks passed
@akien-mga
Copy link
Member

akien-mga commented Aug 7, 2023

Thanks! And congrats for your first merged Godot contribution 🎉

@AttackButton AttackButton deleted the doc-current_scene branch August 7, 2023 13:27
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.

4 participants