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

Added custom node export #67055

Merged
merged 1 commit into from
Oct 31, 2022

Conversation

GuilhermeGSousa
Copy link
Contributor

@GuilhermeGSousa GuilhermeGSousa commented Oct 7, 2022

Small draft PR that aims to implement #4785. I've been testing it for a while and it has been quite a handy feature, here is a small demo:
node_export

Test Export node script

extends Node

@export var x : TestClass

Test Class node script

extends Node

class_name TestClass

Many other PRs have paved the way so that this feature shouldn't be too hard to implement. Currently only the selection using the scene tree popup is done here, the drag/drop of a node of a specific custom class isn't done yet. I'll update this draft once I have it implemented correctly.

Todo (let me know if you see anything missing here):

  • SceneTree selection for custom class export
  • Drag/Drop for custom class export
  • SceneTree selection for arrays of custom classes
  • Drag/Drop for arrays of custom classes

Godot 4 does not have type Dictionary hints according to this comment, so these changes should only address EditorProperty and EditorPropertyArray.

@GuilhermeGSousa
Copy link
Contributor Author

Just realized that according to #62916 exported typed arrays are currently broken (and confirmed on beta 2 while testing these changes), so I don't think there is much more to add to this PR.

#4785 wasn't approved yet so I'll leave this PR as is, but let me know when you want me to "undraft-it" or if I should add anything else to it.

@akien-mga
Copy link
Member

#4785 wasn't approved yet so I'll leave this PR as is, but let me know when you want me to "undraft-it" or if I should add anything else to it.

If it's ready it doesn't need to have draft status. Proposals are not always formally approved before approving a matching PR, sometimes we cut corners :)

@GuilhermeGSousa GuilhermeGSousa marked this pull request as ready for review October 8, 2022 17:17
@GuilhermeGSousa GuilhermeGSousa requested a review from a team as a code owner October 8, 2022 17:17
@GuilhermeGSousa GuilhermeGSousa marked this pull request as draft October 14, 2022 11:19
@GuilhermeGSousa GuilhermeGSousa marked this pull request as ready for review October 14, 2022 11:45
@@ -3696,8 +3696,10 @@ bool EditorPropertyNodePath::is_drop_valid(const Dictionary &p_drag_data) const
return true;
}

StringName type = EditorNode::get_singleton()->get_object_custom_type_name(dropped_node);
Copy link
Member

Choose a reason for hiding this comment

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

This seems unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed I'll remove it

@mhilbrunner
Copy link
Member

PR review meeting: looks good at first glance, would be good if @reduz and @willnationsdev can have a look

@GuilhermeGSousa
Copy link
Contributor Author

@vnen could I get your review for this one?

@reduz
Copy link
Member

reduz commented Oct 27, 2022

Its probably ideal that @willnationsdev reviews this at this point in the current absence of @vnen.

@willnationsdev
Copy link
Contributor

I just checked this out now. The basic testing I've done of the feature all seems to work just fine. However, there is an edge case that isn't accounted for. Not sure if it's within the scope of this PR or not though.

Say you have a Base, DerivedA, and DerivedB, along with an initially exported property of static type DerivedA. You assign a DerivedA node to it, but then modify the script with the property so that it exports DerivedB instead. Currently, the scene retains the reference to the node despite the fact that it now violates the exported property type. How do we resolve the now conflicting inheritance hierarchy? Conceptually it's simple; it should be updated with a null value. However, implementation-wise, I'm unsure of the best way to implement it. The scene file isn't necessarily going to be open in the Godot Editor when the script's exported property's static type hint is updated, but the Editor will need to update it (and every other scene with a now-conflicting exported node) regardless.

Exported resources should've had to deal with this for a long time already I would think. How is the scene's serialized data updated in that scenario? Can that apply here?

Copy link
Contributor

@willnationsdev willnationsdev left a comment

Choose a reason for hiding this comment

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

See previous comment.

@GuilhermeGSousa
Copy link
Contributor Author

@willnationsdev Good point, thought that is an issue already present with built-in type exports, I get that same behaviour if I export with type Camera2D, assign it then change the export type to something else like Sprite2D. I'd argue it could be done in another PR.

It does seem to be managed correctly with exported custom resources though, so we should probably be able to do the same for nodes. After taking a quick look into the editor_resource_picker code the type seems to be checked when the EditorResourcePicker is updated, the same could be done for nodes.

Let me know if you want me to do that here or on a separate PR.

@GuilhermeGSousa GuilhermeGSousa requested review from willnationsdev and removed request for akien-mga October 28, 2022 13:56
Copy link
Contributor

@willnationsdev willnationsdev left a comment

Choose a reason for hiding this comment

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

@GuilhermeGSousa

Good point, thought that is an issue already present with built-in type exports, I get that same behaviour if I export with type Camera2D, assign it then change the export type to something else like Sprite2D. I'd argue it could be done in another PR.

If that edge case is already the current behavior regardless of whether the node is a scripted type or not, then it sounds like it should be dealt with by an entirely separate PR. I'm inclined to approve this one.

It does seem to be managed correctly with exported custom resources though, so we should probably be able to do the same for nodes.

My thoughts too.

After taking a quick look into the editor_resource_picker code the type seems to be checked when the EditorResourcePicker is updated, the same could be done for nodes.

While that might work, it wouldn't fundamentally fix the problem. That would only update inline whatever objects are opened in the Inspector. You'd have to manually click on every erroneous node in the scene, re-save the scene, and then linearly move on to opening the next erroneous scene manually. It wouldn't globally and automatically update all of the project's scene files that have corrupted data integrity.

Whatever solution we use would likely involve a pre-existing mechanism for resources to recursively update in response to changes in their dependency hierarchy relative to other resources in the project. Something like... 1) save a Script, triggers notifications on all resources depending on that Script, 2) for each of those resources, they determine whether or not they need to update themselves. If so, then they trigger notifications on things depending on them, 3) etc.

@GuilhermeGSousa
Copy link
Contributor Author

@willnationsdev sounds good, I'll open an issue for that specific case then

@akien-mga akien-mga merged commit f4f98c4 into godotengine:master Oct 31, 2022
@akien-mga
Copy link
Member

Thanks!

@gelvinp
Copy link
Contributor

gelvinp commented Nov 5, 2022

It looks like this isn't quite working with tool scripts. I'm testing this to export the initial state for a node based state machine, and it's working (launching the game has the player in the correct initial state), but I'm also using _get_configuration_warnings() to check if the initial state is set or not, and throwing a print statement in there displays the initial_state variable to be <null>, even though it's set in the editor.

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.

6 participants