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
Labels
kind/enhancement New feature or request requires/documentation Cannot be merged, requires documentation.
Milestone

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.

@TBNEO TBNEO added the kind/enhancement New feature or request label Apr 18, 2024
@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?
  • OScript needs to store a reference/pointer to the base OScript parent type.
  • OScriptVirtualMachine::call_method needs to handle iterating the OScript class hierarchy.

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

@Naros Naros added the requires/documentation Cannot be merged, requires documentation. label Apr 21, 2024
@Naros Naros modified the milestones: 2.1, 2.2 Jul 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request requires/documentation Cannot be merged, requires documentation.
Projects
None yet
Development

No branches or pull requests

2 participants