[GDnative] add pluginscript \o/ #11953

touilleMan · 2017-10-08T22:38:58Z

@karroffel This code is not final so don't merge it yet, I'm posting it to have more eyes on it ;-)

There is still some commented stuff and TODO that need to be discuss about but the code works fine with godot-python.

neikeq · 2017-10-08T22:43:06Z

Wow, is this that DLScript thing everyone is talking about?

karroffel

Really cool!

karroffel · 2017-10-08T22:45:09Z

modules/gdnative/include/pluginscript/godot_pluginscript.h

+	//  flags: <int>
+	//  rpc_mode: <int:godot_method_rpc_mode>
+	// }
+	godot_array methods;


If you know the format, why not use a struct godot_pluginscript_method_desc with those fields, so you can have a godot_pluginscript_method_desc *methods; // pointer to array and num of methods? should be less painfull than filling a dictionary.

But a dictionary works too of course.

It seems easier for me to build those fields with godot arrays&dict to avoid painful memory management (typically having to free a single array vs having to walk all the sub arrays and fields in each elements to free everything by hand...)

karroffel · 2017-10-08T22:45:42Z

modules/gdnative/include/pluginscript/godot_pluginscript.h

+	//  rpc_mode: <int:godot_method_rpc_mode>
+	// }
+	godot_array methods;
+	// Same format than for methods


Same format as for methods ;)

karroffel · 2017-10-08T22:49:39Z

modules/gdnative/pluginscript/pluginscript_instance.cpp

+}
+
+ScriptLanguage *PluginScriptInstance::get_language() {
+	return this->_script.ptr()->get_language();


Ref<T> has an overloaded operator->, you don't need that .ptr()->, just use this->_script->get_language();

This happens in multiple places.

karroffel · 2017-10-08T22:57:10Z

modules/gdnative/pluginscript/pluginscript_instance.cpp

+}
+
+Variant PluginScriptInstance::call(const StringName &p_method, const Variant **p_args, int p_argcount, Variant::CallError &r_error) {
+	// TODO: optimize when calling a Godot method from Godot to avoid param conversion ?


I know it's in a TODO, but the conversion is really just an act of tricking the type system and shouldn't have any performance impacts.

It's not about Godot's String -> godot_string conversion I'm talking about but (example with godot-python) godot_string -> Python str -> godot_string when you call from Godot a method of a Python Script and this method is in fact defined in a parent implemented within Godot

karroffel · 2017-10-08T22:59:48Z

modules/gdnative/pluginscript/pluginscript_language.cpp

+
+Script *PluginScriptLanguage::create_script() const {
+	PluginScript *script = memnew(PluginScript());
+	// I'm hurting kittens doing this I guess...


karroffel · 2017-10-08T23:03:39Z

modules/gdnative/pluginscript/pluginscript_language.cpp

+	if (this->_desc.profiling_start) {
+		this->_desc.profiling_start();
+	}
+	this->unlock();


I would move the locking inside the if. If the scripting language doesn't provide profiling methods then why even bother locking? There won't be any changes anyway.

karroffel · 2017-10-08T23:08:21Z

modules/gdnative/include/pluginscript/godot_pluginscript.h

+
+// --- Language ---
+
+typedef struct {


Why doesn't language have a void *data; field?

karroffel · 2017-10-08T23:11:55Z

modules/gdnative/pluginscript/pluginscript_script.cpp

+}
+
+void PluginScript::update_exports() {
+	// TODO


Yup, that's pretty important. :P

I'm not sure what should be done here, so I would prefer to keep it this way for the current PR ;-)

karroffel · 2017-10-08T23:13:31Z

modules/gdnative/pluginscript/pluginscript_script.cpp

+Error PluginScript::reload(bool p_keep_state) {
+	this->_language->lock();
+	ERR_FAIL_COND_V(!p_keep_state && this->_instances.size(), ERR_ALREADY_IN_USE);
+	this->_language->unlock();


Won't this be useless in release builds?

But in release reload is called once at load time, so I guess performance is not really a concern (beside GDscript doesn't remove this code in release).

So if you're sure about this removal, I'll do it ;-)

Yeah you're right about that, no need to remove

karroffel · 2017-10-08T23:20:01Z

modules/gdnative/pluginscript/register_types.cpp

+	if (ret) {
+		ERR_FAIL();
+	}
+	// TODO: @Karroffel how should we pass the langage_desc argument ? Should we only keep a pointer inside PluginScriptLanguage ?


That's a good question. It depends on if the struct's lifetime is greater than that of the PluginScriptLanguage object aka heap or static data. Should make up a convention for that.

My current guess is it's pretty easy for user's library to have a static struct of godot_pluginscript_language_desc (or store the structure in the godot_pluginscript_language_data structure that is currently missing as you pointed out ^^), so we should be able to use the passed pointer during the entire lifetime of the program. The thing is we should add a comment or use a convention to tell the user he should not free this thing until godot_pluginscript_language_desc.finish is called

This comment can be removed

HummusSamurai · 2017-10-09T05:18:07Z

Closes #8505 ?

touilleMan · 2017-10-09T05:52:32Z

@neikeq yep, now nothing can stop Brainfuck to be the number 1 language for Godot !

groud · 2017-10-12T13:55:14Z

Can someone explain me what is PR this about ? I don't understand what is it for.

27thLiz · 2017-10-12T14:54:36Z

@groud this is for adding "plug & play" script languages via GDNative.
It abstracts away some of the inconviences of GDNative.
If a language is exposed as a PluginScript it will benefit from the same editor integration as GDScript/C#.
So you can download the plugin from the assetlib and are able to choose that language directly in the "Add script" dialog, have the buildsystem set up, use the godot script editor, able to use templates, build automatically when running the game, etc.. all the things you'd have to do manually when using GDNative bindings directly.

touilleMan · 2017-10-12T15:43:57Z

modules/gdnative/include/pluginscript/godot_pluginscript.h

+	//     self_time: <int:microseconds>
+	//   }
+	// }
+	int (*profiling_get_accumulated_data)(godot_pluginscript_language_data *p_data, godot_dictionary *r_info, int p_info_max);


I'm not sure about how ProfilingInfo is passed here:

In the end pluginscript will fill an a C array of ProfilingInfo structs.

Currently the language will fill a godot dict, with function signature as key and a dict of ints as value.

So Pluginscript iterate over the dict to fill the final C array. Meaning:

creating the dict with function signature as key takes time for each insertion for no benefit (given the dict is only iterated)

creating all the subdicts is costly

the iteration & recopy of the dicts into the ProfilingInfo class instances takes time

Given profiling_get_frame_data is called for each frame during debug, I think we should try to lower the overhead, the possibilities I see:

replace the godot dict returned by the function by an godot array

create a new C struct equivalent ProfilingInfo and allocate an array of them to be filled by the profiling_get_frame_data

pass the actual C array of ProfilingInfo class instances to profiling_get_frame_data, and allow it to be filled with a custom function:

void godot_pluginscript_fill_profiling_entry(void *profiling_array, int index, godot_string_name *signature, godot_int call_count, godot_int total_time, godot_int self_time) { ProfilingInfo *array = profiling_array; array[index].signature = StringName(*(StringName*)signature); array[index].call_count = call_count; array[index].total_time = total_time; array[index].self_time = self_time; }

@karroffel any idea on this ?

touilleMan · 2017-10-12T15:50:43Z

modules/gdnative/pluginscript/pluginscript_instance.cpp

+
+bool PluginScriptInstance::set(const StringName &p_name, const Variant &p_value) {
+	String name = String(p_name);
+	return this->_desc->set_prop(this->_data, (const godot_string *)&name, (const godot_variant *)&p_value);


As you may have noticed, I'm a python guy so I love to access my instance's attribute explicitly (e.g. this->_attribute) ^^

This is not what is done in the rest of the project so I can update this to a more traditional implicit this, just tell me if it hurts you ;p

It doesn't exactly hurt me, but I'd still prefer to remove the this pointers. ^^

Yeah, if only for consistency, it's better not to use this pointers when not necessary.

If you prefer I can also provide a PR to put this-> everywhere in Godot codebase

akien-mga · 2017-10-13T07:47:18Z

modules/gdnative/pluginscript/pluginscript_instance.cpp

@@ -0,0 +1,151 @@
+// Godot imports


Don't forget to add the Godot copyright header to all .cpp and .h files.

HummusSamurai · 2017-10-14T05:31:58Z

@groud

Can someone explain me what is PR this about ? I don't understand what is it for.

I believe it's an implementation of #8505

touilleMan · 2017-10-14T07:36:10Z

@akien-mga @karroffel do you think we could have this merged for the next alpha ?

Only thing on my todo list remaining are #11953 (comment) and #11937 (comment) which I should be able to do this weekend

touilleMan · 2017-10-15T06:55:34Z

Build system is corrected to work like arvrnative one

touilleMan · 2017-10-15T23:59:46Z

@akien-mga @karroffel #11953 (comment) and #11937 (comment) are done now, it's up to you now ;-)

akien-mga · 2017-10-16T08:09:41Z

Could you squash some of the commits together to merge the fixups in the commits introducing the code that had to be fixed up? 15 commits is a lot, but not all are relevant to the git history.

(And I can't check the travis log, but it still seems to be failing, probably style issue?)

touilleMan · 2017-10-16T18:11:21Z

@akien-mga squashed and style corrected ;-)

karroffel · 2017-10-16T18:28:08Z

modules/gdnative/register_types.cpp

@@ -208,6 +210,7 @@ void unregister_gdnative_types() {
 	singleton_gdnatives.clear();

 	unregister_nativearvr_types();
+	unregister_pluginscript_types();
 	unregister_nativescript_types();


Oh while you're here, can you make the order of unregistering the opposite of registering? @BastiaanOlij put the nativearvr stuff in the wrong order 😛

reduz · 2017-10-17T11:15:36Z

🎉 🎉 🎉 🎈 🍾

touilleMan · 2017-10-17T12:12:30Z

vnen · 2017-11-19T04:11:48Z

I have missed this merge. Is there any documentation about how to use this?

karroffel · 2017-11-19T10:18:16Z

@vnen not yet, but all that this PR does is provide one header with data structures and only one C function, godot_pluginscript_register_language. If that function gets called from a GDNative singleton (a library that gets loaded at startup and stays loaded all the time) then you can provide a new scripting language, as if it was done via a module.

In order to implement the scripting language you need to fill out the struct you pass to godot_pluginscript_register_language accordingly with function pointers and other information.

touilleMan · 2017-11-19T18:14:33Z

@vnen I guess best "documentation" so far for pluginscript is godot-python 😜

If you need more specific questions info, don't hesitate to ping me on the discord

neikeq added feature proposal topic:gdextension labels Oct 8, 2017

neikeq added this to the 3.0 milestone Oct 8, 2017

karroffel suggested changes Oct 8, 2017

View reviewed changes

touilleMan force-pushed the pluginscript branch from 1251ead to 3cb6af8 Compare October 9, 2017 06:19

touilleMan commented Oct 12, 2017

View reviewed changes

touilleMan force-pushed the pluginscript branch 2 times, most recently from 97ca109 to 4546385 Compare October 12, 2017 18:52

akien-mga reviewed Oct 13, 2017

View reviewed changes

touilleMan force-pushed the pluginscript branch from d96753c to dd84fc5 Compare October 15, 2017 06:32

touilleMan force-pushed the pluginscript branch from 8f61184 to dbbb5a0 Compare October 15, 2017 23:58

touilleMan force-pushed the pluginscript branch from dbbb5a0 to cecd8af Compare October 16, 2017 18:10

karroffel reviewed Oct 16, 2017

View reviewed changes

[GDnative] add pluginscript \o/

e715be0

touilleMan force-pushed the pluginscript branch from cecd8af to e715be0 Compare October 17, 2017 08:50

karroffel merged commit 8c50bc3 into godotengine:master Oct 17, 2017

HummusSamurai mentioned this pull request Oct 17, 2017

Extensions for Godot #8505

Closed

[GDnative] add pluginscript \o/ #11953

[GDnative] add pluginscript \o/ #11953

Conversation

touilleMan commented Oct 8, 2017

neikeq commented Oct 8, 2017

karroffel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

touilleMan Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HummusSamurai commented Oct 9, 2017

touilleMan commented Oct 9, 2017

groud commented Oct 12, 2017

27thLiz commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HummusSamurai commented Oct 14, 2017

touilleMan commented Oct 14, 2017 • edited Loading

touilleMan commented Oct 15, 2017

touilleMan commented Oct 15, 2017

akien-mga commented Oct 16, 2017

touilleMan commented Oct 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reduz commented Oct 17, 2017

touilleMan commented Oct 17, 2017

vnen commented Nov 19, 2017

karroffel commented Nov 19, 2017

touilleMan commented Nov 19, 2017

touilleMan Oct 9, 2017 •

edited

Loading

touilleMan commented Oct 14, 2017 •

edited

Loading