Feat/ergonomics 2.0 #65

Benjamin-Dobell · 2025-03-27T19:17:18Z

Firstly, I apologize, this is a mammoth PR. It's somewhat split up into commits, but honestly, this was implemented with lots of experimentation. I only came back and tried to organize things after the fact.

There's an example project which utilizes all of these features at https://github.com/Benjamin-Dobell/GodotJS-Card3D

Some of the camel-case API binding implementation is a bit ugly, particularly in the codegen file.

Camel-case Godot APIs

There's now a project setting which can be toggled to swap to a more idiomatic JS naming scheme for Godot bindings. We use camel and pascal case to more closely align with typical JavaScript/TypeScript conventions. For @Decorators we've gone with pascal case, which is used in libraries like Angular. Camel case is perhaps more popular, but pascal case allows us to avoid reserved names and thus we can cleanly write @Export, instead of needing to include the trailing underscore on @export_.

Basically this brings us in-line with the Mono/C# Godot build, whereby the APIs are idiomatic for .NET.

You can view the camel-case API bindings at https://github.com/Benjamin-Dobell/GodotJS-Card3D/tree/main/typings

Improved types for Signal, Callable etc.

TypeScript types have been improved. Particularly Signal<> and Callable<>. Signal1, Signal2, etc. are now deprecated as are AnySignal and AnyCallable, since the Signal and Callable types now handle an arbitrary number of parameters. Importantly, Callable.bind(...) is now accurately typed, so you'll receive type errors when connecting to signals.

Ex.

Consider two signals with different callback signatures.

@ExportSignal()
mouseExitDropZone!: Signal<() => void>;

@ExportSignal()
cardSelected!: Signal<(selected: Card) => void>;

We can omit the generic parameter from Signal to permit any signal irrespective of callback parameters, similar to the now deprecated AnySignal:

Not only do the new generic Signal/Callback types support an unlimited number of parameters, there's an additional advantage is that consumers now have access to parameter names in addition to types:

Let's create a callback and try connect it to a signal:

The above correctly raises an error; our callback has too many parameters for the signal. We can bind our callback to prepopulate the right-most parameter as follows:

Note that our bound callback has not lost its type information. It's a valid callback for cardSelected, but not for mouseExitDropZone.

This works as expected because Callback<T>.bind() is now typed as:

bind<A extends any[]>(...varargs: A): Callable<BindRight<T, A>>

With BindRight implemented as:

type BindRight<F extends (this: any, ...args: any[]) => any, B extends any[]> =
  F extends (this: infer T, ...args: [...(infer A), ...B]) => infer R
    ? (this: T, ...args: A) => R
    : never;

Stuff like BindRight showcases the real power of TypeScript 😄

Type safe GArray and GDictionary from literals

GArray and GDictionary now have a static .create() method which allows you to create nested data structures from literals and benefit from full type checking. When a GArray or GDictionary is expected as a property, a .proxy() can be provided in its place.

Example included below under "Codegen improvements".

GArray/GDictionary proxy improvements

Partially worked around microsoft/TypeScript#43826 whereby our proxied GArray and GDictionary always return proxied nested values, but will accept non-proxied values when mutating a property. Basically there's now GArrayReadProxy and GDictionaryReadProxy, these aren't runtime types, they're just TS types that make it easier to work with proxies. Under normal circumstances, users won't need to know they exist.

Codegen improvements

Codegen leveled up. Any TS module can now export a function named codegen with the type CodeGenHandler. This function will be called during codegen to allow users to optionally augment type generation involving user defined types. Consider for example the SceneNodes codegen which previously only knew how to handle Godot/native types in the scene hierarchy. When a user type was encountered, it'd write the native type, which is still useful, but it'd be nice to be able to include user types. The reason we don't by default is user types are not required to follow our generic parameter convention where each node is passed a Map argument.

Let's see an example:

export default class CardCollection<Card extends CardNode = CardNode> extends GameNode<SceneNodes['scenes/card_collection_3d.tscn']> {

The type above does not take a Map. Perhaps more interesting, it takes a different generic parameter, a CardNode. If we encounter a CardCollection script attached to a node in the scene somewhere GodotJS' internal codegen can't possibly know what that generic parameter ought to be. So we can help it out. In the same file where CardCollection is defined, we could provide a codegen handler like so:

export const codegen: CodeGenHandler = rawRequest => {
  const request = rawRequest.proxy();

  switch (request.type) {
    case CodeGenType.ScriptNodeTypeDescriptor: {
      const cardNodeScript = request.node.get('cardNodeScript');
      return GDictionary.create<UserTypeDescriptor>({
        type: DescriptorType.User,
        name: 'CardCollection',
        resource: 'res://src/card-collection.ts',
        arguments: GArray.create<TypeDescriptor>([
          GDictionary.create<UserTypeDescriptor>({
            type: DescriptorType.User,
            name: cardNodeScript?.getGlobalName() ?? 'CardNode',
            resource: cardNodeScript?.resourcePath ?? 'res://src/card-node.ts',
          }),
        ]),
      });
    }
  }

  return undefined;
};

Above we handle the codegen request to determine the node type of the provided request.node. What's really neat here is we don't need to hard-code that generic. We've instead exported a a configurable Script reference for use in the editor:

@ExportObject(Script)
cardNodeScript: Script = ResourceLoader.load('res://src/card-node.ts') as Script;

So the codegen logic simply grabs the type exported from the chosen script, and provides it as a generic argument to CardCollection<>.

One thing worth noting, your class does NOT need to be a @Tool. In the above example, CardCollection<T> is not a @Tool, and hence the node script is not instantiated during codegen, which is why we've used request.node.get('cardNodeScript') rather than trying to access the property directly. That said, if you want, codegen can be combined with @Tool.

Right now we only have the one type of codegen request. However, in my example above you may have noted I had to cast the return value of ResourceLoader.load() to a Script. That's another area that's ripe for codegen; string literal paths ought to result in a known resource type. If someone doesn't beat me to it, I'll contribute this too.

Type descriptors

The idea with the codegen is you can return a descriptor that represents any possible TypeScript type. Right now, it probably doesn't encompass everything, but it should be close:

https://github.com/Benjamin-Dobell/GodotJS-Card3D/blob/main/typings/jsb.editor.bundle.d.ts#L4-L124

We return this descriptor representation rather than just a string, because the descriptor representation is much easier to manipulate and derive other types from. There's an example of me deriving one node's type from the type of its child nodes.

Improved class / base class detection regex.

It was failing in the presence of generics which contain an extends clause e.g.

export default class GameNode<Map extends Record<string, Node> = {}> extends Node3D<Map>

The regex will now look for the last extends on the line in order to detect the base class. This is only an improvement, it's not fool-proof and will fail if the base class has a generic that contains a conditional type expression. Since we only have access to PCRE2, this is probably the best we can do with just regex.

Fixed a bug in TScopedPointer that leads to runtime crashes.

Exported properties/signals no longer leak into the base class.

Previously sub-classes were reusing the same [[ClassProperties]] and [[ClassSignals]] as super classes. Thus sub-classes were exporting properties against super-classes. This also meant that classes with a shared parent were receiving each others' properties. Each class no longer looks up the prototype chain for these objects.

Consequently, to ensure properties are exported and appear in the Editor, we now recurse in a similar fashion to GDScript. Fortunately, we don't need to worry about the cycle detection logic that GDScript implements, since TypeScript handles this for us and cycles won't compile. Added benefit is now that properties appear in the editor categorized appropriately by class.

There's also a bunch of logging/error reporting improvements.

feat: inherited properties now class categorized in the Editor. Previously sub-classes were reusing the same [[ClassProperties]] and [[ClassSignals]] as super classes. Thus sub-classes were exporting properties against super-classes. This also meant that classes with a shared parent were receiving each others' properties. Each class no longer looks up the prototype chain for these objects. Consequently, to ensure properties are exported and appear in the Editor, we now recurse in a similar fashion to GDScript. Fortunately, we don't need to worry about the cycle detection logic that GDScript implements, since TypeScript handles this for us and cycles won't compile. Added benefit is now that properties appear in the editor categorized appropriately by class.

It was failing in the presence of generics which contain an extends clause e.g. export default class GameNode<Map extends Record<string, Node> = {}> extends Node3D<Map> The regex will now look for the last extends on the line in order to detect the base class. This is only an improvement, it's not fool-proof and will fail if the base class has a generic that contains a conditional type expression. Since we only have access to PCRE2, this is probably the best we can do with just regex.

Benjamin-Dobell · 2025-03-27T19:22:08Z

@ialex32x Just thought I'd give you a heads up that I'm working on this 😅

Benjamin-Dobell · 2025-03-28T14:16:20Z

This is ready for review now. I didn't need to make any changes for release builds. I just had a messed up release build locally; forgot to rebuild the latest JS runtime after swapping branch 😅

ialex32x · 2025-03-31T01:14:11Z

Hello @Benjamin-Dobell, I really appreciate your PR. Could you double-check the changes in jsb_sarrayh since the operator= with move semantic should steal the lock as expected? Is there a minimal scenario to reproduce the crash issue you encountered?

Benjamin-Dobell · 2025-03-31T01:21:08Z

The issue I encountered is with releasing the existing/previous lock, if you reuse a TScopedPointer variable. You want to take on the lock of the moved/assigned pointer, but you want to release whatever lock you previously held. About to head out, but I'll try get you a reproduction in about 8 hours.

ialex32x · 2025-03-28T01:47:44Z

internal/jsb_sarray.h

@@ -142,6 +142,7 @@ namespace jsb::internal
            {
                if (this != &p_other)
                {
+                    if (container_) container_->unlock_address();


In the move assignment, the lock is stolen from p_other, no additional unlock is needed as expected, right?
Will the crash be caused by other factors? For instance, there may have been an unexpected modification during the lock.

ialex32x · 2025-03-28T02:00:25Z

weaver-editor/jsb_editor_helper.cpp

@@ -1,21 +1,112 @@
 #include "jsb_editor_helper.h"

-Dictionary GodotJSEditorHelper::_build_node_path_map(Node *node)
+#include <modules/GodotJS/bridge/jsb_callable.h>


Use relative paths for self module headers to achieve better compatibility with the gdextension build, though it's not implemented yet :P

ialex32x · 2025-03-31T01:33:55Z

bridge/jsb_object_bindings.cpp

-            jsb_throw(isolate, "signal owner is undefined or dead");
+            const String error_message = jsb_errorf("failure obtaining signal: %s. signal owner is undefined or dead", gd_signal_name);
+            impl::Helper::throw_error(isolate, error_message);
+            jsb_throw(isolate, "");


a redundant call to throw

ialex32x · 2025-03-31T01:36:43Z

bridge/jsb_object_bindings.cpp

@@ -324,7 +335,8 @@ namespace jsb
            info.GetReturnValue().Set(jrval);
            return;
        }
-        jsb_throw(isolate, "failed to translate godot variant to v8 value");
+        const String error_message = jsb_errorf("Failed to return from call: %s. Failed to translate returned Godot %s to a JS value", method_bind->get_name(), index, Variant::get_type_name(crval.get_type()));


missing format placeholder for index

ialex32x · 2025-03-31T01:37:53Z

bridge/jsb_object_bindings.cpp

@@ -369,7 +384,9 @@ namespace jsb
            info.GetReturnValue().Set(jrval);
            return;
        }
-        jsb_throw(isolate, "failed to translate godot variant to v8 value");
+        const String error_message = jsb_errorf("Failed to get property: %s. Failed to translate returned Godot %s to a JS value",


missing format placeholder for index

ialex32x · 2025-03-31T01:38:47Z

bridge/jsb_object_bindings.cpp

+            const String error_message = jsb_errorf("Failed to set property: %s. Unable to convert provided JS %s to Godot %s",
+                property_info.setter_func->get_name(), TypeConvert::js_debug_typeof(isolate, info[0]),
+                Variant::get_type_name(property_info.setter_func->get_argument_type(1)));
+            impl::Helper::throw_error(isolate, error_message);


the following jsb_throw after this statement becomes redundant

@export

- There's now a project setting which can be toggle to swap to a more idiomatic JS naming scheme for Godot bindings. We use camel and pascal case to more closely align with typical JavaScript/TypeScript conventions. For @decorators we've gone with pascal case, which is used in libraries like Angular. Camel case is perhaps more popular, but pascal case allows us to avoid reserved names and thus we can cleanly write @export, instead of needing to include the trailing underscore on @export_. - TypeScript types have been improved. Particularly Signal<> and Callable<>. Signal1, Signal2, etc. are now deprecated as are AnySignal and AnyCallable, since the Signal<T> and Callable<T> types now handle an arbitrary number of parameters. Importantly, Callable.bind(...) is now accurately typed, so you'll receive type errors when connecting to signals. - GArray and GDictionary now have a static .create<T>() method which allows you to create nested data structures from literals and benefit from full type checking. When a GArray or GDictionary is expected as a property, a .proxy() can be provided in its place. - Partially worked around microsoft/TypeScript#43826 whereby or proxied GArray and GDictionary always return proxied nested values, but will accept non-proxied values when mutating a property. Basically there's now GArrayReadProxy and GDictionaryReadProxy, these aren't runtime types, they're just TS types that make it easier to work with proxies. Under normal circumstances, users won't need to know they exist. - Codegen leveled up. Any TS module can now export a function named `codegen` with the type CodeGenHandler. This function will be called during codegen to allow users to optionally augment type generation involving user defined types. Consider for example the SceneNodes codegen which previously only knew how to handle Godot/native types in the scene hierarchy. When a user type was encountered, it'd write the native type, which is still useful, but it'd be nice to be able to include user types. The reason we don't by default is user types are not required to follow our generic parameter convention where each node is passed a Map argument. Let's see an example: export default class CardCollection<Card extends CardNode = CardNode> extends GameNode<SceneNodes['scenes/card_collection_3d.tscn']> the type above does not take a Map. Perhaps more interesting, it takes a different generic parameter, a CardNode. If we encounter a CardCollection script attached to a node in the scene somewhere GodotJS' internal codegen can't possibly know what that generic parameter ought to be. So we can help it out. In the same file where CardCollection is defined, we could provide a codegen handler like so: export const codegen: CodeGenHandler = rawRequest => { const request = rawRequest.proxy(); switch (request.type) { case CodeGenType.ScriptNodeTypeDescriptor: { const cardNodeScript = request.node.get('cardNodeScript'); return GDictionary.create<UserTypeDescriptor>({ type: DescriptorType.User, name: 'CardCollection', resource: 'res://src/card-collection.ts', arguments: GArray.create<TypeDescriptor>([ GDictionary.create<UserTypeDescriptor>({ type: DescriptorType.User, name: cardNodeScript?.getGlobalName() ?? 'CardNode', resource: cardNodeScript?.resourcePath ?? 'res://src/card-node.ts', }), ]), }); } } return undefined; }; Above we handle the codegen request to determine the node type of the provided `request.node`. What's *really* neat here is we don't need to hard-code that generic. We've instead exported a a configurable Script reference for use in the editor: @ExportObject(Script) cardNodeScript: Script = ResourceLoader.load('res://src/card-node.ts') as Script; So the codegen logic simply grabs the type exported from the chosen script provides it as a generic argument to CardCollection<>. One thing worth noting, your class does NOT need to be a @tool. In the above example, CardCollection<T> is not a @tool, and hence the node script is not instantiated during codegen, which is why we've used `request.node.get('cardNodeScript')` rather than trying to access the property directly. That said, if you want, codegen can be combined with @tool. Right now we only have the one type of codegen request. However, in my example above you may have noted I had to cast the return value of ResourceLoader.load() to a Script. That's another area that's ripe for codegen; string literal paths ought to result in a known resource type. If someone doesn't beat me to it, I'll contribute this too. - There's also a bunch of logging/error reporting improvements.

Benjamin-Dobell · 2025-04-01T13:24:48Z

Really appreciate the time you've taken to review such a large PR 🙏 I've just force pushed addressing your feedback. Sorry about the delay in doing so. Ended up getting sick. The joys of being a father of young kids 🤦

I've pushed a reproduction for the TScopedPointer crash.

Basically, reassigning (from a temporary TScopedPointer i.e. move assignment) causes the problem:
Benjamin-Dobell@6933f3f#diff-90949b3fc5ace885b7a0d20647770fa0665ac70aa01ef62e457cf38ec735860bL819-R820
This isn't practical code, I'm reassigning the exact same value. It's just for demonstration purposes. If you check-out that commit, Godot will crash on start-up.

This leads to a crash later on because the lock is still held:

I don't believe this PR actually ended up introducing any TScopedPointer move reassignments, I ran into the crash when experimenting with some other potential solutions. But figured I ought to include the fix anyway.

ialex32x · 2025-04-02T03:09:48Z

Really appreciate the time you've taken to review such a large PR 🙏 I've just force pushed addressing your feedback. Sorry about the delay in doing so. Ended up getting sick. The joys of being a father of young kids 🤦

I've pushed a reproduction for the TScopedPointer crash.

Basically, reassigning (from a temporary TScopedPointer i.e. move assignment) causes the problem: Benjamin-Dobell@6933f3f#diff-90949b3fc5ace885b7a0d20647770fa0665ac70aa01ef62e457cf38ec735860bL819-R820 This isn't practical code, I'm reassigning the exact same value. It's just for demonstration purposes. If you check-out that commit, Godot will crash on start-up.

This leads to a crash later on because the lock is still held:

I don't believe this PR actually ended up introducing any TScopedPointer move reassignments, I ran into the crash when experimenting with some other potential solutions. But figured I ought to include the fix anyway.

You're exactly right! It should be unlocked before being replaced.
Please make sure to take care of yourself 🌞.
Many thanks for your contribution!

Benjamin-Dobell added 5 commits March 28, 2025 05:03

fix: ensure TScopedPointer lock is released when reassigned w/ move

256ba62

fix: runtime script typechecking during build

23affea

feat: Improved type conversion error messages

0b3544b

Benjamin-Dobell marked this pull request as ready for review March 28, 2025 14:09

ialex32x reviewed Mar 31, 2025

View reviewed changes

Benjamin-Dobell force-pushed the feat/ergonomics-2.0 branch from 5cca42b to 0f084e7 Compare April 1, 2025 13:09

ialex32x merged commit 7029ca1 into godotjs:main Apr 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/ergonomics 2.0 #65

Feat/ergonomics 2.0 #65

Benjamin-Dobell commented Mar 27, 2025 •

edited

Loading

Benjamin-Dobell commented Mar 27, 2025

Benjamin-Dobell commented Mar 28, 2025

ialex32x commented Mar 31, 2025 •

edited

Loading

Benjamin-Dobell commented Mar 31, 2025

ialex32x Mar 28, 2025

ialex32x Mar 28, 2025

ialex32x Mar 31, 2025

ialex32x Mar 31, 2025

ialex32x Mar 31, 2025

ialex32x Mar 31, 2025

Benjamin-Dobell commented Apr 1, 2025

ialex32x commented Apr 2, 2025

Feat/ergonomics 2.0 #65

Feat/ergonomics 2.0 #65

Conversation

Benjamin-Dobell commented Mar 27, 2025 • edited Loading

Camel-case Godot APIs

Improved types for Signal, Callable etc.

Type safe GArray and GDictionary from literals

GArray/GDictionary proxy improvements

Codegen improvements

Type descriptors

Improved class / base class detection regex.

Fixed a bug in TScopedPointer that leads to runtime crashes.

Exported properties/signals no longer leak into the base class.

There's also a bunch of logging/error reporting improvements.

Benjamin-Dobell commented Mar 27, 2025

Benjamin-Dobell commented Mar 28, 2025

ialex32x commented Mar 31, 2025 • edited Loading

Benjamin-Dobell commented Mar 31, 2025

ialex32x Mar 28, 2025

Choose a reason for hiding this comment

ialex32x Mar 28, 2025

Choose a reason for hiding this comment

ialex32x Mar 31, 2025

Choose a reason for hiding this comment

ialex32x Mar 31, 2025

Choose a reason for hiding this comment

ialex32x Mar 31, 2025

Choose a reason for hiding this comment

ialex32x Mar 31, 2025

Choose a reason for hiding this comment

Benjamin-Dobell commented Apr 1, 2025

ialex32x commented Apr 2, 2025

Benjamin-Dobell commented Mar 27, 2025 •

edited

Loading

ialex32x commented Mar 31, 2025 •

edited

Loading