Allow different class names in rust and Godot #419

fpdotmonkey · 2023-09-21T19:23:40Z

There's an issue that two structs with the same name in different modules will conflict with each other when sent to Godot since only the struct name is considered. This resolves that by allowing the API user to manually change the name of the class as Godot understands it. This also improves the error message that occurs when theres are aliased classes.

Future work may seek to catch this issue at compile time rather than at runtime.

Relevant to #332

GodotRust · 2023-09-21T20:31:02Z

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-419

Bromeon

Thanks a lot!

One thing that went through my head is the string-based syntax #[class(rename="MyClass")], but since it's effectively an identifier in GDScript, I think this is fine!

Bromeon · 2023-09-21T20:30:10Z

godot-core/src/registry.rs

+            )
+            .unwrap_or_else(|_|
+                panic!(
+                    "the definition for `{}` is overloaded; you may have multiple `GodotClass`es named `{}` which you should #[class(rename=...)]",


"overloaded" is a bit imprecise here, but I also think there is no need to repeat the class name.

Maybe simply:

Suggested change

"the definition for `{}` is overloaded; you may have multiple `GodotClass`es named `{}` which you should #[class(rename=...)]",

"class `{}` is already defined; to register equally named structs under different names, use `#[class(rename=...)]`",

I think it would be good to keep GodotClass in there to point the user to that symbol (I guess class is technically the relevant symbol, but it reads as generic here).

I'll take another shot at this error message

This would still need to be updated 🙂

Regarding GodotClass, you could mention:

"class `{}` is already defined; to register equally named structs under\n\ different names, use `#[derive(GodotClass)` with `#[class(rename=...)]`"

(it might deserve the newline)

Bromeon · 2023-09-21T20:31:11Z

godot-macros/src/lib.rs

@@ -318,6 +318,34 @@ use crate::util::ident;
 /// for more information and further customization.
 ///
 /// This is very similar to [GDScript's `@tool` feature](https://docs.godotengine.org/en/stable/tutorials/plugins/running_code_in_the_editor.html).
+///
+///
+/// # Class Name Aliasing


Maybe "Class renaming" -- aliasing is typically associated with &mut.

Bromeon · 2023-09-21T20:33:28Z

godot-macros/src/lib.rs

+/// You may want to have structs with the same name.  With rust, this is
+/// allowed using `mod`.  However in GDScript, there are no modules,
+/// namespaces, or any such diambiguation.  Therefore, you need to
+/// change the names before they can get to Godot.  You can use the
+/// `rename` key while defining your `GodotClass` for this.


Use a bit wider lines if possible (~120 chars).

Also, some typos:

rust -> Rust

diambiguation

no need for double space after full stop

Bromeon · 2023-09-21T20:34:54Z

itest/rust/src/object_tests/class_rename_test.rs

+    use super::*;
+
+    #[derive(GodotClass)]
+    #[class(init, base = Node)]


#[class] could probably be emitted entirely, since neither the constructor nor the base class are relevant.

Bromeon · 2023-09-21T20:35:14Z

itest/rust/src/object_tests/class_rename_test.rs

+    use super::*;
+
+    #[derive(GodotClass)]
+    #[class(init, base = Node, rename = NoRepeat)]


Also here, just #[class(rename = NoRepeat)] would be enough.

Bromeon · 2023-09-21T20:36:45Z

itest/rust/src/object_tests/class_rename_test.rs

+#[itest]
+fn renaming_changes_the_name() {
+    // note: this test doesn't really fail; if it doesn't succeed, then
+    // the runner hangs.  I don't know how to solve that.
+    assert!(dont_rename::RepeatMe::class_name() != rename::RepeatMe::class_name());
+    assert_eq!(dont_rename::RepeatMe::class_name().as_str(), "RepeatMe");
+    assert_eq!(rename::RepeatMe::class_name().as_str(), "NoRepeat");
+}


Just hangs -- do you see any error or so?

First statement could be assert_ne!, by the way 🙂

Hang is maybe imprecise? If you don't rename the class, you hit the error mentioned in the issue and Godot throws some errors and the runner can't proceed

> godot4 --path itest/godot --headless -- \[\] Initialize GDExtension API for Rust: Godot Engine v4.1.1.stable.official Godot Engine v4.1.1.stable.official.bd6af8e0e - https://godotengine.org ERROR: Rust function panicked in file godot-core/src/registry.rs at line 209. Context: failed to initialize GDExtension level `Scene` at: <function unset> (godot-core/src/lib.rs:154) ERROR: Panic msg: the definition for `RepeatMe` is overloaded; you may have multiple `GodotClass`es named `RepeatMe` which you should #[class(rename=...)] at: <function unset> (godot-core/src/lib.rs:102) SCRIPT ERROR: Parse Error: Identifier "IntegrationTests" not declared in the current scope. at: GDScript::reload (res://TestRunner.gd:31)

But I've now discovered that this is solved by not having the init or base in the class declarations as you suggested.

I always forget about assert_ne, I always try assert_neq and when that doesn't work, go for assert

Bromeon · 2023-09-21T20:40:55Z

Another small request: please exclude issue/PR references from commit messages. Unfortunately GitHub is not very smart about them and links even those of abandoned branches permanently, which clutters the issue quite a bit:

fpdotmonkey · 2023-09-21T21:48:50Z

Another small request: please exclude issue/PR references from commit messages. Unfortunately GitHub is not very smart about them and links even those of abandoned branches permanently, which clutters the issue quite a bit

Oh that's very annoying, noted!

Bromeon

Did you push the correct commit? Many of my reviews are still unaddressed 🤔

Maybe check the diff directly, the conversation tab is sometimes not very good to keep an overview...

Bromeon · 2023-09-23T21:20:53Z

godot-core/src/registry.rs

+            )
+            .unwrap_or_else(|_|
+                panic!(
+                    "the definition for `{}` is overloaded; you may have multiple `GodotClass`es named `{}` which you should #[class(rename=...)]",


This would still need to be updated 🙂

Regarding GodotClass, you could mention:

"class `{}` is already defined; to register equally named structs under\n\ different names, use `#[derive(GodotClass)` with `#[class(rename=...)]`"

(it might deserve the newline)

Bromeon · 2023-09-23T21:29:57Z

godot-macros/src/class/derive_godot_class.rs

@@ -21,7 +21,7 @@ pub fn derive_godot_class(decl: Declaration) -> ParseResult<TokenStream> {
    let fields = parse_fields(class)?;

    let class_name = &class.name;
-    let class_name_str = class.name.to_string();
+    let class_name_str = struct_cfg.rename.unwrap_or(class.name.clone()).to_string();


This always executes both clone() and to_string().

map_or_else() or a good old if let Some(...) would be a bit nicer 🙂

Bromeon · 2023-09-24T07:51:12Z

Sorry about the conflicts, merging #393 caused some changes.

fpdotmonkey · 2023-09-28T14:49:51Z

Did you push the correct commit?

Sorry about that, forgot to stage most of the changes, woops. Should be good now, assuming the CI approves,

Bromeon · 2023-09-29T08:29:22Z

Thank you! The CI errors now, in dictionary_test, seem unrelated to this. Will look into it...

Bromeon · 2023-09-30T08:35:28Z

itest/rust/src/object_tests/class_rename_test.rs

+    // note: this test doesn't really fail; if it doesn't succeed, then
+    // the runner hangs.  I don't know how to solve that.


Can you update this description?

There's an issue that two structs with the same name in different modules will conflict with each other when sent to Godot since only the struct name is considered. This resolves that by allowing the API user to manually change the name of the class as Godot understands it. This also improves the error message that occurs when there are aliased classes. Future work may seek to catch this issue at compile time rather than at runtime.

fpdotmonkey force-pushed the alias-class-definitions branch from 2ab8845 to 352f84a Compare September 21, 2023 20:29

Bromeon linked an issue Sep 21, 2023 that may be closed by this pull request

Classname Alias #332

Closed

Bromeon reviewed Sep 21, 2023

View reviewed changes

Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Sep 21, 2023

fpdotmonkey force-pushed the alias-class-definitions branch from 352f84a to b82d645 Compare September 21, 2023 22:10

fpdotmonkey requested a review from Bromeon September 23, 2023 16:56

Bromeon reviewed Sep 23, 2023

View reviewed changes

fpdotmonkey force-pushed the alias-class-definitions branch 2 times, most recently from d403c4c to 83ca3e1 Compare September 28, 2023 14:48

Bromeon reviewed Sep 30, 2023

View reviewed changes

Bromeon force-pushed the alias-class-definitions branch 2 times, most recently from 83ca3e1 to e98d16f Compare September 30, 2023 08:39

fpdotmonkey force-pushed the alias-class-definitions branch from e98d16f to 6e07b1d Compare October 3, 2023 17:03

Bromeon added this pull request to the merge queue Oct 3, 2023

Merged via the queue into godot-rust:master with commit 88a7934 Oct 3, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow different class names in rust and Godot #419

Allow different class names in rust and Godot #419

fpdotmonkey commented Sep 21, 2023

GodotRust commented Sep 21, 2023

Bromeon left a comment

Bromeon Sep 21, 2023

fpdotmonkey Sep 21, 2023

Bromeon Sep 23, 2023

Bromeon Sep 21, 2023 •

edited

Loading

Bromeon Sep 21, 2023

Bromeon Sep 21, 2023

Bromeon Sep 21, 2023

Bromeon Sep 21, 2023

fpdotmonkey Sep 21, 2023

fpdotmonkey Sep 21, 2023

fpdotmonkey Sep 21, 2023

Bromeon commented Sep 21, 2023

fpdotmonkey commented Sep 21, 2023

Bromeon left a comment •

edited

Loading

Bromeon Sep 23, 2023

Bromeon Sep 23, 2023

Bromeon commented Sep 24, 2023

fpdotmonkey commented Sep 28, 2023

Bromeon commented Sep 29, 2023

Bromeon Sep 30, 2023

	"the definition for `{}` is overloaded; you may have multiple `GodotClass`es named `{}` which you should #[class(rename=...)]",
	"class `{}` is already defined; to register equally named structs under different names, use `#[class(rename=...)]`",

		// note: this test doesn't really fail; if it doesn't succeed, then
		// the runner hangs. I don't know how to solve that.

Allow different class names in rust and Godot #419

Allow different class names in rust and Godot #419

Conversation

fpdotmonkey commented Sep 21, 2023

GodotRust commented Sep 21, 2023

Bromeon 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

Bromeon Sep 21, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Sep 21, 2023

fpdotmonkey commented Sep 21, 2023

Bromeon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Sep 24, 2023

fpdotmonkey commented Sep 28, 2023

Bromeon commented Sep 29, 2023

Choose a reason for hiding this comment

Bromeon Sep 21, 2023 •

edited

Loading

Bromeon left a comment •

edited

Loading