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 NodePath Hint to specify a list of acceptable Node Types #38402

Closed
wants to merge 2 commits into from
Closed

Added NodePath Hint to specify a list of acceptable Node Types #38402

wants to merge 2 commits into from

Conversation

fnaranjo-vmw
Copy link

Added the ability to specify a list of valid types when exporting a NodePath (also works for CustomTypes)
This functionality was already working in VisualScript so I simply filled the gaps to make it work in GDScript, and C#.

By specifying a list of valid types the NodeSelector dialog will only allow selecting nodes of those types. Other nodes in the tree will still appear but will be disabled (grayed out.)

It has been tested to also work for NodePath Arrays in GDScript.
It is not working in C# for IEnumerable types though.
However it seems that export hints are never applied for C# IEnumerable types (it appears to be a bug and is not in the scope of this change.)

HOW IT WORKS:
In GDScript:
(Simply enumerate the type of the Nodes that can be selected)
export(NodePath, "Button", "CustomType") var nodepath
You are not forced to specfify any type. This still works:
export(NodePath) var nodepath

In C#:
[Export(PropertyHint.NodePathValidTypes,"Button,CustomType")] private Node2D nodepath;
(Notice that in C# the list of valid types must be encoded as a single String)

Some requests related to this change:

@fnaranjo-vmw fnaranjo-vmw requested review from bojidar-bg, vnen and a team as code owners May 2, 2020 00:00
@sampengilly
Copy link
Contributor

This seems pretty useful, is it possible to use static references to the class though instead of strings?

@fnaranjo-vmw
Copy link
Author

fnaranjo-vmw commented May 2, 2020

It seems perfectly possible in GDScript.

Having a static reference to the class in C# doesn't seem trivial, because of the way Export Hints are defined in C#.

Also, because of the cross-language interoperability it may make a lot of sense to use strings (or at least to have both options).
The class may be defined in a different language, for example, and therefore not easily available as a Static Reference for the current language of the script.

I would suggest to include this implementation and possibly improve over it later to also support static references.

My reasons to suggest this are the following:

  • Apparently, having a static reference to the class name doesn't prevent the code from breaking if the class changes its name (Godot doesn't perform autorenaming. I'm I right?)
  • I don't think it brings much tipe-safety to the table since in the end it is still a NodePath we will be operating with. Therefore, lots of this functionality is still resolved at runtime.
  • It makes sense to have a common cross-language approach, all based on Strings.
    Supporting a more type safe way in other languages too (VisualScript, C#) will be very useful but may require a lot more work.

I also understand that this functionality, once merged, should not change drastically breaking backward compatibility.

So I'm open to suggestions.
I can add Static Class references in GDScript, but now I'm in doubt of we should then keep the String based alternative in GDScript.

What do you think?

@Shadowblitz16
Copy link

Shadowblitz16 commented May 6, 2020

so wait C# already has a type system are you saying we aren't going to be able to export custom node types?

what would be ideal would something like..
export(Sprite) var my_sprite
[Export] Sprite sprite
of course it would be a reference to a node not a node path but would allow you to choose existing nodes in the scene like a nodepath

@neikeq
Copy link
Contributor

neikeq commented May 21, 2020

C# is missing a few custom export modes so don't worry about this. At some point I'll have to implement them.

@creikey
Copy link
Contributor

creikey commented Jul 5, 2020

I dislike this string based implementation as in large projects it can become tedious to have to list a base type as a string and all of its inherited types, the way I am implementing it in a WIP PR that would fully resolve godotengine/godot-proposals#1048 is passing any Script as its resource path to the scene_tree_editor and using get_base_script in the Script virtual object to find if the script's path matches any of its inherited types' paths

@fnaranjo-vmw
Copy link
Author

Hello @creikey
Some time has passed since I created this PR but as far as I can remember inherited types would be selectable without explicitly listing them.

I agree with you that the string approach is far from ideal but if you take a look at the code you will see that my main goal was:

  • introduce as few changes as possible
  • make this feature work the same across all languages
    (Tested for Gdscript, C#, VisualScript)
  • Have this working as soon as possible (with the hope of improving it in future major releases)

@creikey
Copy link
Contributor

creikey commented Jul 5, 2020

@fejnartal The native type works when matching however scripts that extend from other scripts ( fairly common in bigger games ) are not recognized from the base script

e.x: adding "CustomClass" as an export hint in the nodepath assign dialog
image

@fnaranjo-vmw
Copy link
Author

Hi @creikey thank you for testing that and answering.
I will take a look and see if I can fix that with some minor changes.

The solution will still be based on strings I'm afraid.
My little experience in C++ and Godot Engine internals won't allow me to try a more sofisticated approach.
It would also need much more work and major changes to add stronger typesafety to all major supported languages.

@Calinou
Copy link
Member

Calinou commented Jul 8, 2020

Judging by the last screenshot in this article, exporting specific node types in NodePath will be possible in Godot 4.0:

@fnaranjo-vmw
Copy link
Author

Thank you @Calinou
Yes, it seems so. And it looks great :D!
But apparently this is part of a major GDScript rewrite, so I guess this feature won't be available in C# nor VisualScript out of the box.

godotengine/godot-proposals#828

I guess it won't hurt if I can try to add missing functionality identified by @creikey to this PullRequest since it may be a good starting point for other developments even if this proves to be insufficient or not as useful as I though it would be.

I currently don't have a proper development environment. at hand I will try to compile and test this change myself as soon as I have one.
But just in case any of you have the time and interest to try to test it yourself I have added a new check that may work for custom types' subclasses too as @creikey has suggested.

https://docs.godotengine.org/es/stable/classes/class_classdb.html#class-classdb-method-get-inheriters-from-class
@fnaranjo-vmw
Copy link
Author

I currently don't have a proper development environment at hand. I will try to compile and test this change myself as soon as I have one.
But just in case any of you have the time and interest to try to test it yourself I have added a new check that may work for custom types' subclasses too as @creikey has suggested.

https://docs.godotengine.org/es/stable/classes/class_classdb.html#class-classdb-method-get-inheriters-from-class

@Calinou Calinou added this to the 3.2 milestone Jan 5, 2021
@aaronfranke aaronfranke modified the milestones: 3.2, 3.3 Mar 16, 2021
@akien-mga akien-mga modified the milestones: 3.3, 3.4 Mar 26, 2021
@akien-mga
Copy link
Member

The original repository has been deleted and there's another similar PR for that feature #39155, so I'll close this one.

@akien-mga akien-mga closed this Mar 26, 2021
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.

9 participants