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.

Sign up for GitHub

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

Jump to bottom

Add Node.get_tree_string and Node.get_tree_string_pretty #77072

Merged
merged 1 commit into from
Oct 5, 2023
Merged

Add Node.get_tree_string and Node.get_tree_string_pretty #77072

merged 1 commit into from
Oct 5, 2023

Conversation

vPumpking
Copy link
Contributor

@vPumpking vPumpking commented May 14, 2023 •
edited by Calinou
Loading

add get_tree_string() and get_tree_string_pretty to Node class

see #73421 for details

@vPumpking vPumpking requested review from a team as code owners May 14, 2023 16:05
@vPumpking vPumpking mentioned this pull request May 14, 2023
Closed
@vPumpking
Copy link
Contributor Author

I will soon resolve the conflicts

@fire fire changed the title added get_tree_string() to Node class Added get_tree_string() to Node class May 15, 2023
@vPumpking vPumpking changed the title Added get_tree_string() to Node class Add get_tree_string() to Node class May 15, 2023
@akien-mga akien-mga added this to the 4.x milestone May 15, 2023
@vPumpking
Copy link
Contributor Author

vPumpking commented May 21, 2023 •
edited
Loading

All conflicts are resolved! It's ready to be verified and merged!
Edit: on my repo ( https://github.com/vPumpking/godot/tree/get-tree-string ), the workflows are making errors but I don't know why. If someone knows, please tell me

AThousandShips
AThousandShips reviewed May 21, 2023
View reviewed changes
scene/main/node.cpp Outdated
new_prefix = last ? String::utf8(" ") : String::utf8(" ┃ ");
data.children_cache[i]->_print_tree_pretty(prefix + new_prefix, i == data.children_cache.size() - 1);
return_tree += data.children[i]->_get_tree_string_pretty(prefix + new_prefix, i == data.children.size() - 1);
Copy link
Member

@AThousandShips AThousandShips May 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You need to use children_cache as in the current source, children is a HashMap not a vector

When rebasing make sure to check the current state of the code to make sure you work with how it works currently, I would recommend rewriting your additions onto the current code to make sure you don't miss anything

AThousandShips
AThousandShips reviewed May 21, 2023
View reviewed changes
scene/main/node.cpp
String new_prefix = last ? String::utf8(" ┖╴") : String::utf8(" ┠╴");
print_line(prefix + new_prefix + String(get_name()));
_update_children_cache();
Copy link
Member

@AThousandShips AThousandShips May 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This line also has to be kept, and added to the other functions that examine the tree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what other functions?

Copy link
Member

Choose a reason for hiding this comment

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

_get_tree_string in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

line 2153 only or both 2153 and 2152?

Copy link
Member

@AThousandShips AThousandShips May 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Only the second, you have to update the children cache

You should have seen some of these when doing the rebase and resolving conflicts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no sorry I forgot something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no sorry I forgot something

it depens
should I replace children by children_cache in all the 2 functions or only on these 2 lines?

Copy link
Member

Choose a reason for hiding this comment

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

Look at the original code

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 think it's done
(actually compiling local)

@AThousandShips
Copy link
Member

Also I really recommend compiling the code locally to be able to do updates to documentation and test the functionality

AThousandShips
AThousandShips reviewed May 21, 2023
View reviewed changes
scene/main/node.cpp Outdated
for (uint32_t i = 0; i < data.children_cache.size(); i++) {
String return_tree = "";
return_tree += prefix + new_prefix + String(get_name()) + "\n";
for (int i = 0; i < data.children.size(); i++) {
Copy link
Member

@AThousandShips AThousandShips May 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You're still using children here, see the code you're replacing, just keep the original line here to get it correct

Copy link
Member

Choose a reason for hiding this comment

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

As I said above, I'd recommend restoring all the original code and then adding your changes again to avoid overwriting or ignoring important code

AThousandShips
AThousandShips reviewed May 21, 2023
View reviewed changes
AThousandShips
AThousandShips previously approved these changes May 21, 2023
View reviewed changes
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Other than the wording in the documentation it looks good to me

@AThousandShips AThousandShips dismissed their stale review September 11, 2023 17:58

Reviewed before membership, will re-review

AThousandShips
AThousandShips approved these changes Sep 11, 2023
View reviewed changes
Copy link
Member

@AThousandShips AThousandShips 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 save for the following

@vPumpking
Copy link
Contributor Author

done

AThousandShips
AThousandShips reviewed Sep 13, 2023
View reviewed changes
@vPumpking
Copy link
Contributor Author

(really) done

@YuriSizov YuriSizov changed the title Add get_tree_string() to Node class Add Node.get_tree_string() Sep 14, 2023
@YuriSizov YuriSizov changed the title Add Node.get_tree_string() Add Node.get_tree_string and Node.get_tree_string_pretty Sep 14, 2023
@YuriSizov
Copy link
Contributor

Could you please squash your commits into one? Make sure that the final commit has a short but descriptive message (the title of this PR is a good option). See this documentation, if you need help with squashing.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Sep 14, 2023
@vPumpking
Copy link
Contributor Author

I think it's ok

@YuriSizov
Copy link
Contributor

It's squashed, but you still need to update the message. And it would also be a good idea to remove extra lines left from squashing from the commit's description:

Fix: missing cache updates in _get_tree_string

Fix: missing cache updates in _get_tree_string and variants
fix


fix


Code optimisation

Delete useless lines
Update Documentation for Node

Update definition of get_tree_string and get_tree_string_pretty

@vPumpking
Copy link
Contributor Author

It's squashed, but you still need to update the message. And it would also be a good idea to remove extra lines left from squashing from the commit's description:

Fix: missing cache updates in _get_tree_string

Fix: missing cache updates in _get_tree_string and variants
fix


fix


Code optimisation

Delete useless lines
Update Documentation for Node

Update definition of get_tree_string and get_tree_string_pretty

how can I do that?
I mean, updating the commit's description

@AThousandShips
Copy link
Member

git commit --amend

@YuriSizov
Copy link
Contributor

You can use git commit --amend -m "your commit message goes here" on your latest commit. Or, if you're more familiar with the interactive rebase workflow (which you've used to squash the commits), then you can use the r or reword directive to change the commit message.

@vPumpking
Copy link
Contributor Author

vPumpking commented Sep 15, 2023 •
edited
Loading

is it better like this?

@YuriSizov
Copy link
Contributor

Yep, it's fine now!

@vPumpking
Copy link
Contributor Author

sorry for asking, but can you estimate the time remaining until the PR is merged? I'm new (hello, New) and I have no idea on that

YuriSizov
YuriSizov approved these changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Juan approved ™

@akien-mga akien-mga merged commit 3b70e30 into godotengine:master Oct 5, 2023
@vPumpking vPumpking deleted the get-tree-string branch October 5, 2023 15:42
@vPumpking
Copy link
Contributor Author

fuck

@vPumpking
Copy link
Contributor Author

I clicked the wrong button

@vPumpking
Copy link
Contributor Author

TvT

@vPumpking
Copy link
Contributor Author

@YuriSizov does the fact I deleted the branch affects the merge?

@YuriSizov
Copy link
Contributor

@vPumpking Calm down! 🙃 It has been merged an hour ago, so you are free to delete your branch now. Also, thanks and congrats on your first merged PR!

@vPumpking vPumpking restored the get-tree-string branch October 16, 2023 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AThousandShips AThousandShips AThousandShips approved these changes

@mhilbrunner mhilbrunner mhilbrunner approved these changes

@YuriSizov YuriSizov YuriSizov approved these changes

Assignees
No one assigned
Labels
feature proposal topic:core
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.

Add get_tree_string() and get_tree_string_pretty() methods to Node to complement printing methods
6 participants
@vPumpking @AThousandShips @YuriSizov @mhilbrunner @Calinou @akien-mga