Add NativeScript support for script classes via exported script properties. #20561

willnationsdev · 2018-07-29T07:34:27Z

Depends on #20560.

This enables NativeScripts to be registered as script classes by having the script in question implement String-returning methods _get_script_class_name() and _get_script_class_icon_path().

Let me know what you guys think.

willnationsdev · 2018-07-29T07:36:58Z

@mysticfall This is an example of an implementation of those methods. The C# one would just have to parse out the relevant info from its GDMonoClass stuff (I think, idk all the C# details).

VisualScript would probably need you to add the name/icon_path as properties of VisualScript itself and then create GUI elements in the visual script editor that let the user input values for the name/icon, possibly use a FileDialog to select the desired icon, etc. Then the VisualScriptLanguage implementation would be a trivial task of loading the VisualScript and returning the member variables.

Not sure about PluginScript.

karroffel · 2018-07-31T07:13:55Z

I think we can add an API to NativeScript 1.1 to get rid of that function call

willnationsdev · 2018-07-31T11:58:43Z

@karroffel In that case, you'll have to explain how the NativeScript class would access that information (to return it in the get_global_class_name method). Member property?

karroffel · 2018-07-31T12:05:06Z

@willnationsdev we can add a new field to the gdns resource called global_name and the read from there without having to actually load a library.

willnationsdev · 2018-07-31T13:11:13Z

@karroffel That'd be good. We'd also need a property for a file path to an icon.

willnationsdev · 2018-08-05T05:11:43Z

@karroffel I just modified this PR, so let me know what you think. I made properties "script_class_name" and "script_class_icon_path". Going to sleep now. ;-)

karroffel · 2018-08-05T06:47:28Z

modules/gdnative/nativescript/nativescript.cpp

 	ClassDB::bind_method(D_METHOD("get_class_documentation"), &NativeScript::get_class_documentation);
 	ClassDB::bind_method(D_METHOD("get_method_documentation", "method"), &NativeScript::get_method_documentation);
 	ClassDB::bind_method(D_METHOD("get_signal_documentation", "signal_name"), &NativeScript::get_signal_documentation);
 	ClassDB::bind_method(D_METHOD("get_property_documentation", "path"), &NativeScript::get_property_documentation);

 	ADD_PROPERTYNZ(PropertyInfo(Variant::STRING, "class_name"), "set_class_name", "get_class_name");
 	ADD_PROPERTYNZ(PropertyInfo(Variant::OBJECT, "library", PROPERTY_HINT_RESOURCE_TYPE, "GDNativeLibrary"), "set_library", "get_library");
+	ADD_GROUP("Script Class", "script_class");


Isn't it common to include the trailing _ here? Not a deal breaker and the editor probably still shows it correctly 😄

karroffel · 2018-08-05T07:00:16Z

modules/gdnative/nativescript/nativescript.cpp

+}
+
+String NativeScriptLanguage::get_global_class_name(const String &p_path, String *r_base_type, String *r_icon_path) const {
+	Ref<NativeScript> script = ResourceLoader::load(p_path, "NativeScript");


This actually loads and initializes the library. That's the only way you can get information like "base type", but it's still suboptimal.

This is just a comment, you don't need to change anything here, it's fine, just realized that it's a bit "meh" to load and unload libraries all the time when all you need is three strings 😄

Is there perhaps a better way we should be defining / accessing these values then? Perhaps some way to directly access the data contained within the resource file itself? I mean, I could directly open up the file and parse it for the information...but that's kinda hacky.

Actually, if it inherits Resource, is there a way to load it AS a Resource, rather than as a NativeScript, and then just directly access the member properties on it using .get() (assuming that doesn't also load the library)?

I don't think so, if you just parse it as a regular resource it will dispatch to the custom code anyway AFAIK. And yeah, there's information like "base type" which you only know at runtime, unless we make the user specify it in the .gdns file as well (would only be necessary for global classes then)

karroffel · 2018-08-05T07:01:03Z

modules/gdnative/nativescript/nativescript.cpp

 	ClassDB::bind_method(D_METHOD("get_class_documentation"), &NativeScript::get_class_documentation);
 	ClassDB::bind_method(D_METHOD("get_method_documentation", "method"), &NativeScript::get_method_documentation);
 	ClassDB::bind_method(D_METHOD("get_signal_documentation", "signal_name"), &NativeScript::get_signal_documentation);
 	ClassDB::bind_method(D_METHOD("get_property_documentation", "path"), &NativeScript::get_property_documentation);

 	ADD_PROPERTYNZ(PropertyInfo(Variant::STRING, "class_name"), "set_class_name", "get_class_name");
 	ADD_PROPERTYNZ(PropertyInfo(Variant::OBJECT, "library", PROPERTY_HINT_RESOURCE_TYPE, "GDNativeLibrary"), "set_library", "get_library");
+	ADD_GROUP("Script Class", "script_class");


Isn't it common to include the trailing _ here? Shouldn't make a difference in the editor, just wondering 😄

karroffel · 2018-08-15T15:19:13Z

reeeeebase :)

willnationsdev · 2018-08-15T15:27:29Z

Lol, gotcha. I'll get right on that in about an hour

karroffel

Looks good and can be merged, but this will probably make some underlying bugs (leaks) more annoying. This is probably a good thing so I have a reason to fix them more timely :)

willnationsdev · 2018-08-16T17:58:32Z

@karroffel Ok, cool. I don't have the ability to merge things, so someone else will have to do that. ;-)

willnationsdev requested review from bojidar-bg, reduz and vnen as code owners July 29, 2018 07:34

willnationsdev force-pushed the script-class-ns branch 2 times, most recently from abba1cf to b1626f1 Compare July 31, 2018 02:59

willnationsdev force-pushed the script-class-ns branch 3 times, most recently from 3113f84 to 5e4a48f Compare August 5, 2018 04:58

karroffel reviewed Aug 5, 2018

View reviewed changes

willnationsdev force-pushed the script-class-ns branch from 5e4a48f to 2daf4de Compare August 9, 2018 03:53

akien-mga added feature proposal topic:gdextension labels Aug 14, 2018

akien-mga added this to the 3.1 milestone Aug 14, 2018

willnationsdev force-pushed the script-class-ns branch from 2daf4de to e151476 Compare August 14, 2018 14:03

Add NativeScript support for script classes.

05f7173

willnationsdev force-pushed the script-class-ns branch from e151476 to 05f7173 Compare August 15, 2018 15:58

willnationsdev changed the title ~~Add NativeScript support for script classes via virtual methods.~~ Add NativeScript support for script classes via exported script properties. Aug 15, 2018

karroffel approved these changes Aug 16, 2018

View reviewed changes

akien-mga merged commit 968b31e into godotengine:master Aug 16, 2018

willnationsdev deleted the script-class-ns branch August 16, 2018 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NativeScript support for script classes via exported script properties. #20561

Add NativeScript support for script classes via exported script properties. #20561

willnationsdev commented Jul 29, 2018

willnationsdev commented Jul 29, 2018

karroffel commented Jul 31, 2018

willnationsdev commented Jul 31, 2018 •

edited

Loading

karroffel commented Jul 31, 2018

willnationsdev commented Jul 31, 2018

willnationsdev commented Aug 5, 2018 •

edited

Loading

karroffel Aug 5, 2018

karroffel Aug 5, 2018

willnationsdev Aug 7, 2018

willnationsdev Aug 7, 2018 •

edited

Loading

karroffel Aug 7, 2018

karroffel Aug 5, 2018

karroffel commented Aug 15, 2018

willnationsdev commented Aug 15, 2018

karroffel left a comment

willnationsdev commented Aug 16, 2018

Add NativeScript support for script classes via exported script properties. #20561

Add NativeScript support for script classes via exported script properties. #20561

Conversation

willnationsdev commented Jul 29, 2018

willnationsdev commented Jul 29, 2018

karroffel commented Jul 31, 2018

willnationsdev commented Jul 31, 2018 • edited Loading

karroffel commented Jul 31, 2018

willnationsdev commented Jul 31, 2018

willnationsdev commented Aug 5, 2018 • edited Loading

karroffel Aug 5, 2018

Choose a reason for hiding this comment

karroffel Aug 5, 2018

Choose a reason for hiding this comment

willnationsdev Aug 7, 2018

Choose a reason for hiding this comment

willnationsdev Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

karroffel Aug 7, 2018

Choose a reason for hiding this comment

karroffel Aug 5, 2018

Choose a reason for hiding this comment

karroffel commented Aug 15, 2018

willnationsdev commented Aug 15, 2018

karroffel left a comment

Choose a reason for hiding this comment

willnationsdev commented Aug 16, 2018

willnationsdev commented Jul 31, 2018 •

edited

Loading

willnationsdev commented Aug 5, 2018 •

edited

Loading

willnationsdev Aug 7, 2018 •

edited

Loading