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

Allow renaming child nodes in _ready #78706

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

RedworkDE
Copy link
Member

@RedworkDE RedworkDE requested a review from a team as a code owner June 26, 2023 08:33
@Calinou Calinou added bug topic:core cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Jun 26, 2023
@Calinou Calinou added this to the 4.x milestone Jun 26, 2023
@akien-mga akien-mga modified the milestones: 4.x, 4.2 Jun 26, 2023
@akien-mga
Copy link
Member

@reduz seemed to like that idea when proposed by @kleonc in #77610 (comment). CC @Sauermann

Agree with keeping this for 4.2 and possibly a cherrypick for 4.1.1 to lift the restriction, so we don't take risks now.

@RedworkDE RedworkDE force-pushed the node-rename-inplace branch 3 times, most recently from 339f6d8 to 62cfff2 Compare June 26, 2023 09:15
@reduz
Copy link
Member

reduz commented Jun 26, 2023

Looks good, great approach!

Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

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

Overall the code looks good to me. 👍 AFAICT should be good as is, but left some notes.

Comment on lines +370 to +373
SWAP(hashes[next_pos], hashes[pos]);
SWAP(elements[next_pos], elements[pos]);
Copy link
Member

Choose a reason for hiding this comment

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

AFAICT only the other entries need to be "shifted by one", no point to keep moving the element we're removing (the last entry in the chain is cleared right after this loop anyway).

Suggested change
SWAP(hashes[next_pos], hashes[pos]);
SWAP(elements[next_pos], elements[pos]);
hashes[pos] = hashes[next_pos];
elements[pos] = elements[next_pos];

(under the assumption that pos != next_pos; which should be the case as next_pos = fastmod((pos + 1), capacity_inv, capacity))


I see in HashMap::erase it's done like that too:

SWAP(hashes[next_pos], hashes[pos]);
SWAP(elements[next_pos], elements[pos]);

It could be changed the same way if the element being removed would be stored just like in here before doing all the "shifting" (HashMapElement<TKey, TValue> *element = elements[pos]) and it would be used instead of elements[pos] in:
if (elements[pos]->prev) {
elements[pos]->prev->next = elements[pos]->next;
}
if (elements[pos]->next) {
elements[pos]->next->prev = elements[pos]->prev;
}
element_alloc.delete_allocation(elements[pos]);

Copy link
Member Author

Choose a reason for hiding this comment

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

This is copied as-is from erase and would rather keep it simple for this PR, same for the duplicate hash computation below (also some operations can compute the hash / lookup the element 3 or 4 times, so hash map really deserves a dedicated optimization PR with measurements etc)

Copy link
Member

Choose a reason for hiding this comment

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

Agree. As mentioned, these are just some notes/thoughts. 🙃

scene/main/node.cpp Outdated Show resolved Hide resolved

// Update the HashMapElement with the new key and reinsert it.
const_cast<TKey &>(element->data.key) = p_new_key;
uint32_t hash = _hash(p_new_key);
Copy link
Member

Choose a reason for hiding this comment

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

This is already calculated in _lookup_pos(p_new_key, pos) so there's a room for optimizing it.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jul 21, 2023
@YuriSizov YuriSizov merged commit 4d42d6f into godotengine:master Jul 21, 2023
@RedworkDE RedworkDE deleted the node-rename-inplace branch July 21, 2023 15:17
@YuriSizov
Copy link
Contributor

Thanks!

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.

7 participants