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

Support for creating classes #261

Open
TBNEO opened this issue Apr 18, 2024 · 1 comment
Open

Support for creating classes #261

TBNEO opened this issue Apr 18, 2024 · 1 comment

Comments

@TBNEO
Copy link

TBNEO commented Apr 18, 2024

Description

The idea is simple: Addition of creating classes and such within an orchestration, similarly to how, in GDScript, one can add the class_name [name here] at the top.
The reasoning behind why do this is mainly for ease of use in inheritance systems. Currently, there isn't an easy way (that i know of) to make an orchestration extend from another. Example,
Player.os inherits movement capabilities from: Entity.os
However, this can't be done at this moment right now.

Implementation ideas

In implementation, this could be solved by adding perhaps a bool for whether it's to be class or not, along with a necessary Class Name field, and this could be done in the script inspector for ease.
Doing this with nodes would be inefficient, I think, but if that's easier, then it works too.

Either way, thanks for reading this, good luck with the project.

@Naros Naros added this to the 2.1 milestone Apr 20, 2024
@Naros
Copy link
Member

Naros commented Apr 21, 2024

So here are some findings after looking into what is required to make this work:

  • Add serialized properties for class_name and icon
  • OScript::_get_global_name should return the class_name
  • OScript::_get_class_icon_path should return the icon
  • OScriptLanguage::_get_global_class_name needs to be implemented (more below)
  • OScriptLanguage::_get_type should return OScript rather than Orchestrator (more below)
  • For an object to show up in Create New Node dialog, cannot extend abstract classes, i.e., CanvasItem.
    • _This seriously looks like a bug, as it's legal to extend CanvasItem even in GDScript.
  • OScript::_get_method_info should be implemented to return method details for GDScript context look-ups.
  • OScript::_has_static_method should be implemented to expose static methods to GDScript (out of scope).
  • OScript::_get_documentation should be implemented for context-help like GDScript (can be follow-up).
  • OScript::_get_script_property_list should be implemented?

So one issue that I don't particularly care for is how Godot resolves the global class name, specifically in the call to the language function _get_global_class_name. This call relies on opening the script, parsing the contents, only to return the following:

Dictionary result;
result["name"] = script->get_global_name();
result["base_type"] = script->get_instance_base_type();
result["icon_path"] = script->get_class_icon_path();

This specifically creates cyclic resource inclusion failures because the resource subsystem identifies that another resource is loaded with the specific paths of res://<script-filename-and-path>::<uid>. GDScript gets around this because instead, the raw parser that converts the text-based source to a list of AST nodes is used, which does not involve the resource subsystem, avoiding this problem. This raises the question of whether the parse step of OScript files should be separate from the resource loader/savers or whether what we're doing needs changes, a lot of this code is undocumented in the engine and text-based scripts are handled differently from our visual-based scripts😞 .

But all things considered, this should be straightforward once we get past the cyclic issue. As a teaser, here's the mock of a simple Control orchestration script:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants