Add new notification when a new Node is created in editor - #81217
Add new notification when a new Node is created in editor#81217Malcolmnixon wants to merge 1 commit into
Conversation
|
Seems like I have been doing this in my game like I suppose this made it so these defaults are enforced every single time the scene is opened, while your PR adds a way to enforce a default only when the node is added? |
|
I'm concerned about adding an editor-specific notification to Node, but this is likely useful and we do seem to have a couple other editor notifications in there. You probably need to add this same notification to |
cc9a6e9 to
f925245
Compare
|
Think the documentation should clarify that it happens when created through the UI, and not through for example a script |
f925245 to
f0e007f
Compare
|
Good catch @YuriSizov - I hadn't considered the other methods of creating node instances via favorites or the file-system dock. @AThousandShips - I updated the node documentation to clarify the notification is only invoked when creating nodes through the Godot editor UI. |
|
I had considered other methods for allowing scripts to override default values; however one of my primary goals was to limit the performance impact on shipping games. The three approaches I considered were:
|
|
cc @vnen What is your view on this George? Is this the proper way with this, it's something we need to continue some of the development on XR Tools for. |
|
If it does solve the problem, I think this implementation is fine. |
Added NOTIFICATION_EDITOR_CREATED notification. Added emitting NOTIFICATION_EDITOR_CREATED when editor creates new nodes
f0e007f to
1365ede
Compare
I added firing NOTIFICATION_EDITOR_CREATED for the Ctrl script-drag into the scene tree. |
|
Asked reduz to review, but in typical reduz fashion he replied to me instead of commenting here ;) |
|
There isn't any |
I guess @reduz was referring to |
Not really. And as I said, this notification is received when node is duplicated, instantiated etc. It's not suitable for initializing a default value, because it would be overwriting the property every time. It's basically early |
|
Just to bring this back to attention. Discussing this during the XR meeting where we are waiting for this functionality. Let's focus back on the original problem we are trying to solve here. Right now it is not possible to extend a class and provide alternative default values for properties from the class we are extending. So RigidBody3D has a default collision layer of 1, in our extended class we want that default layer to be 3. We can do this when we also defined a The bottom line here is solving that issue, this PR solves that by introducing a notification after our node is created, but before our properties are processed, so that the default value can be overridden. The other suggestions made here as alternatives do not fit that purpose, they either happen to early, too late, or too often in order to solve the underlying issue. |
|
I would prefer we find another way to do this than hacking a notification inside Node. As I mentioned above, there are other ways to work around this issue. I would appreciate more detail into this scenario, I still don´t quite understand why it can´t be done via the usual channels. |
|
Regarding use cases, check the bottom of the description of #88647. I tried to implement a virtual method for this, but the method is never being called: https://github.com/Calinou/godot/tree/editor-add-on-instantiated-call |
|
I'm getting reminded about this PR once every few months, because new scenarios keep popping up where such notification would be useful.
This change does not modify node in any way, other than registering a new constant. The notification could be registered elsewhere, but being next to other editor-specific notifications is more intuitive I think.
None of the existing methods/notifications allow to achieve the same thing (unless I'm missing something), so what are these other ways? Again, the goal is to notify the node when it's just created in the editor, once. It's similar to |
There was a problem hiding this comment.
I guess it might be appropriate to move all calls to instantiate_object_properties() instead, it's called in the same scenarios.
|
Superseded by #122058 |
This pull request implements godotengine/godot-proposals#7593 by:
The following script demonstrates how these changes can create a new node script capable of being instantiated via the "Add New Node" scene-tree command and having the node pre-populated with the overridden default values specified by the script.
Bugsquad edit: Closes godotengine/godot-proposals#7593