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

Implement inherits_script() for NativeScript and PluginScript #51180

Merged
merged 1 commit into from
Aug 9, 2021

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Aug 2, 2021

Just that. Now only PluginScript is missing an implementation.

@akien-mga
Copy link
Member

CC @touilleMan to help with PluginScript.

Note that neither NativeScript nor PluginScript are likely usable in the master branch (and IIRC they'll likely both be removed once the extensions system is ready to replace them). But this implementation is needed for #51166 in the 3.x branch so it can be developed for 3.x and just merged in master to future proof.

@touilleMan
Copy link
Member

regarding NativeScript::inherits_script() implementation, it seems a.inherits_script(b) means Does script b inherits script a ? which seems weird (given you read the method a inherits script b).

(my 2 cents: this method should be renamed or at least have some comments near it definition to explain in which order it is supposed to work... I the current state I personally can't say if it implementation is expected or not 😄 )

Regarding PluginScript implementation, base script information is provided during Script creation:

StringName *base_name = (StringName *)&manifest.base;
if (*base_name) {
if (ClassDB::class_exists(*base_name)) {
_native_parent = *base_name;
} else {
Ref<Script> res = ResourceLoader::load(*base_name);
if (res.is_valid()) {
_ref_base_parent = res;
} else {
String name = *(StringName *)&manifest.name;
FREE_SCRIPT_MANIFEST(manifest);
ERR_FAIL_V_MSG(ERR_PARSE_ERROR, _path + ": Script '" + name + "' has an invalid parent '" + *base_name + "'.");
}
}
}

So I think we should be able to implement inherits_script by using the PluginScript._ref_base_parent field in a similar fashion NativeScript::inherits_scripts is implemented.

However I wonder why this need to be implemented on in each Script, it feels much simpler to have one default implementation in Script relying on Script::get_base_script (we would need to introduce a Script::get_script_class_name which already exists in NativeScript, PluginScript and GDScript). Of course this would mean script names must be unique but I guess it's already the case given we register them in ClassDB anyway.

@RandomShaper RandomShaper changed the title Implement NativeScript::inherits_script() Implement inherits_script() for NativeScript and PluginScript Aug 2, 2021
@RandomShaper
Copy link
Member Author

Thank you very much, @touilleMan. I've fixed the inverted logic and implemented the function for PluginScript, both being based on the GDScript implementation.

Now, regarding the approach of doing it by name without having to have an implementation per type of script, I believe that makes sense and would also allow to do checks in hierarchies mixing different scripting languages, as long as that can happen.

However, maybe that would be better separately discussed. If the implementations I've added here are good, that would already be unblocking my current work.

Copy link
Member

@touilleMan touilleMan left a comment

Choose a reason for hiding this comment

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

lgtm

@akien-mga akien-mga merged commit fa1a66d into godotengine:master Aug 9, 2021
@akien-mga
Copy link
Member

Thanks!

@RandomShaper RandomShaper deleted the native_script_inherits branch August 9, 2021 09:57
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.

3 participants