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

Rename NOTIFICATION_INSTANCED to NOTIFICATION_SCENE_INSTANTIATED #64410

Merged

Conversation

MewPurPur
Copy link
Contributor

Looks like we missed an instance of the old "instanced" verb

@MewPurPur MewPurPur requested review from a team as code owners August 14, 2022 22:54
@MewPurPur MewPurPur changed the title Renamed "instanced" notification to "instantiated" Rename "instanced" notification to "instantiated" Aug 14, 2022
@Calinou Calinou added this to the 4.0 milestone Aug 14, 2022
@YuriSizov
Copy link
Contributor

Regarding keeping the terms in sync, this makes sense. But I'm not sure if this is a good name for the notification, and the description is just incorrect. Nodes don't get instantiated, packed scenes do. So there was another proposal for a rename from @QbieShay some time ago, trying to address that problem: #25538.

Maybe we should still consider a better name for this notification.

@MewPurPur
Copy link
Contributor Author

Didn't know about this – should I edit this PR?

@YuriSizov YuriSizov changed the title Rename "instanced" notification to "instantiated" Rename NOTIFICATION_INSTANCED to NOTIFICATION_INSTANTIATED Aug 15, 2022
@YuriSizov
Copy link
Contributor

YuriSizov commented Aug 15, 2022

You can definitely update the description of the notification to indicate that it is received when the node is at the root of a scene that was just instantiated. Basically, the reverse of what the instantiate() description tells.

As for the notification itself, probably more opinions are required. I don't have a preference. Can suggest something like NOTIFICATION_SCENE_INSTANTIATED maybe. But we can also keep it as is in this PR.

@akien-mga
Copy link
Member

If it's received only by scene owners, then yeah SCENE_INSTANTIATED makes sense to me.

@MewPurPur MewPurPur force-pushed the rename-notification-instanced branch from 81a883e to 2599710 Compare August 16, 2022 10:41
@MewPurPur MewPurPur changed the title Rename NOTIFICATION_INSTANCED to NOTIFICATION_INSTANTIATED Rename NOTIFICATION_INSTANCED to NOTIFICATION_SCENE_INSTANTIATED Aug 16, 2022
@akien-mga akien-mga merged commit 889c522 into godotengine:master Aug 30, 2022
@akien-mga
Copy link
Member

Thanks!

@MewPurPur MewPurPur deleted the rename-notification-instanced branch September 1, 2022 20:56
@raulsntos raulsntos mentioned this pull request Sep 8, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Approved
Development

Successfully merging this pull request may close these issues.

4 participants