Support `#[export(range = (radians_as_degrees, suffix="XX"))]` #783

fpdotmonkey · 2024-06-27T22:23:52Z

radians_as_degrees was added in Godot 4.2 to replace radians with identical functionality and a better name. suffix was added in 4.0 and was seemingly missed when #[export(range)] was first implemented.

This is the first instance where Godot has deprecated an API and gdext has needed to implement it. Hopefully this will serve as a useful template for future work.

This also fixes a previously unknown bug where a KEY=VALUE expression in the third slot of range = (...) would yield strange error messages.

Resolves #731

godot-macros/src/class/data_models/field_export.rs

GodotRust · 2024-06-27T22:31:43Z

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

Dheatly23 · 2024-06-28T05:00:38Z

godot-macros/src/class/data_models/field_export.rs

        radians: bool,
        degrees: bool,
        hide_slider: bool,
+        suffix: Option<String>,


Can't you just store the TokenStream and convert it later? Kinda weird that min, max, and step are all expression but suffix isn't.

Yes, that seems to save one to_string call and solves a question I had

Bromeon

Thanks a lot!

Could you document the new keys, e.g. here?
https://godot-rust.github.io/docs/gdext/pr-783/godot/register/property/export_info_functions/fn.export_range.html

I know others don't have docs, but #[export(range = ...)] is particularly complex and has a syntax that isn't used anywhere else. So it would be good to show examples of different possible invocations.

Bromeon · 2024-07-08T18:38:44Z

godot-macros/src/class/data_models/field_export.rs

+                    quote! {
+                    #export_func;
+                    ::godot::__deprecated::emit_deprecated_warning!(export_range_radians);
+                            }


Indentation

This is what rustfmt produces. It wouldn't pass CI if it weren't like this.

Are you sure? Rustfmt is typically rather conservative when it comes to formatting the inside of macro invocations (as in, often doesn't format at all). That's also the value proposition of tools like Gene Michaels.

$ rustfmt --check godot-macros/src/class/data_models/field_export.rs Diff in /home/fp/3rd/gdext/godot-macros/src/class/data_models/field_export.rs at line 402: }?; let deprecation_warning = if *radians { quote! { - #export_func; - ::godot::__deprecated::emit_deprecated_warning!(export_range_radians); - } + #export_func; + ::godot::__deprecated::emit_deprecated_warning!(export_range_radians); + } } else { quote! { #export_func } }; $ rustfmt --version rustfmt 1.7.0-stable (129f3b9 2024-06-10)

Weirdly, this doesn't produce any diff for me, despite being seemingly the same.

impl Something { fn produce_ugly_diff() { match something { AnonymouStructVariant { f0, f1, f2, f3, f4, f5, f6, f7, f8, f9, f10, } => { let warning = if *radians { quote! { #export_func; ::godot::__deprecated::emit_deprecated_warning!(export_range_radians); } } else { quote! { #export_func } }; } } } }

My hunch though is that it has something to do with the anonymous struct variant since we need to turn off a lint disallowing large numbers of parameters. But clearly it's not so easy to replicate.

Assuming it's a bug, I've reported it rust-lang/rustfmt#6233

Thanks a lot for reporting! 👍

So for now you can leave the bad formatting, maybe add a comment that links to this conversation:

https://github.com/godot-rust/gdext/pull/783#discussion_r1669105958

Bromeon · 2024-07-08T18:51:55Z

godot-macros/src/util/list_parser.rs

+    pub(crate) fn next_allowed_key_optional_value(
+        &mut self,
+        allowed_bool_keys: &[&str],
+        allowed_kv_keys: &[&str],
+    ) -> ParseResult<Option<(Ident, Option<KvValue>)>> {
+        let allowed_keys = || {
+            let allowed_bool_keys = allowed_bool_keys.join(",");
+            let allowed_kv_keys = allowed_kv_keys.join(",");
+            [allowed_bool_keys, allowed_kv_keys].join(",")
+        };
+        match self.next_key_optional_value()? {
+            Some((key, None)) if !allowed_bool_keys.contains(&key.to_string().as_str()) => {
+                if allowed_kv_keys.contains(&key.to_string().as_str()) {
+                    return bail!(key, "`{key}` requires a value `{key} = VALUE`");
+                }
+                bail!(key, "expected one of \"{}\"", allowed_keys())
+            }
+            Some((key, Some(_))) if !allowed_kv_keys.contains(&key.to_string().as_str()) => {
+                if allowed_bool_keys.contains(&key.to_string().as_str()) {
+                    return bail!(key, "key `{key}` mustn't have a value");
+                }
+                bail!(key, "expected one of \"{}\"", allowed_keys())
+            }
+            key_maybe_value => Ok(key_maybe_value),
+        }
+    }


This is quite similar to the top-level KvParser logic,with the main difference that "keys" (standalone) are now "values". I guess there isn't really anything to reuse?

Not really, no. And if I did, it'd look a lot like this.

Bromeon · 2024-07-08T18:53:56Z

godot-macros/src/util/list_parser.rs

+    pub(crate) fn next_allowed_key_optional_value(
+        &mut self,
+        allowed_bool_keys: &[&str],
+        allowed_kv_keys: &[&str],
+    ) -> ParseResult<Option<(Ident, Option<KvValue>)>> {


Would be good to keep terminology consistent everywhere:

"flag/flags" for standalone boolean keys

"kv/kvs" for key-value pairs

As for the method name, the "optional" is a bit unclear (I know it refers to "value", but had to read multiple times). Maybe it's not needed given the return type has already Option? So, next_allowed_flag_or_kv might be a different way to state it... or next_allowed_key_value.

Is "allowed" needed at all?

I'll use flags, sure.

So the though behind the name was that it's doing the same thing as next_key_optional_value, but with the possible flag names and kv keys specified, similar to next_allowed_ident(allowed_ids). So combining these two names, I landed on what's here. I agree that the name is a lot to parse, but I think it's reasonable to a user of this struct. An API user here mostly likely will be learning the API by reading the source and will first discover those methods I'm basing this name on before this one. It might be reasonable to rethink the naming, but I don't think that's in scope for this PR.

The outer Option is Some when there is a value to parse and None after then end of the sequence, similar to std::iter::Iterator::next(). If a not-allowed ident is a key, it'll return Err, so the Option type doesn't come into play. This is consistent with next_allowed_ident.

So you're right that this is a bit funny, but it's because I'm combining a couple of existing funny things; I don't think I added any funniness. I will add a docstring to clarify the function, though.

So the though behind the name was that it's doing the same thing as next_key_optional_value, but with the possible flag names and kv keys specified, similar to next_allowed_ident(allowed_ids). So combining these two names, I landed on what's here.

That makes sense, in that case you can keep it. Thanks for elaborating!

I will add a docstring to clarify the function, though.

Great 👍

`radians_as_degrees` was added in Godot [4.2][1] to replace `radians` with identical functionality and a better name. `suffix` was added in [4.0][2] and was seemingly missed when `#[export(range)]` was first implemented. This is the first instance where Godot has deprecated an API and gdext has needed to implement it. Hopefully this will serve as a useful template for future work. This also fixes a previously unknown bug where a `KEY=VALUE` expression in the third slot of `range = (...)` would yield strange error messages. [1]: godotengine/godot#82195 [2]: godotengine/godot#50009

Bromeon · 2024-07-09T18:08:22Z

Thank you!

fpdotmonkey commented Jun 27, 2024

View reviewed changes

godot-macros/src/class/data_models/field_export.rs Outdated Show resolved Hide resolved

Dheatly23 reviewed Jun 28, 2024

View reviewed changes

fpdotmonkey force-pushed the export-range-suffix branch from 8b6e322 to 4cbbdd4 Compare June 28, 2024 07:20

Bromeon added feature c: register labels Jun 28, 2024

fpdotmonkey force-pushed the export-range-suffix branch from 4cbbdd4 to c85b6a3 Compare June 28, 2024 07:30

Bromeon reviewed Jul 8, 2024

View reviewed changes

fpdotmonkey force-pushed the export-range-suffix branch 4 times, most recently from 5eee4e8 to 53e8b78 Compare July 9, 2024 16:17

fpdotmonkey force-pushed the export-range-suffix branch from 53e8b78 to d1a4fee Compare July 9, 2024 16:21

Bromeon added this pull request to the merge queue Jul 9, 2024

Merged via the queue into godot-rust:master with commit 79edae3 Jul 9, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `#[export(range = (radians_as_degrees, suffix="XX"))]` #783

Support `#[export(range = (radians_as_degrees, suffix="XX"))]` #783

fpdotmonkey commented Jun 27, 2024

GodotRust commented Jun 27, 2024

Dheatly23 Jun 28, 2024

fpdotmonkey Jun 28, 2024

Bromeon left a comment

Bromeon Jul 8, 2024

fpdotmonkey Jul 9, 2024

Bromeon Jul 9, 2024

fpdotmonkey Jul 9, 2024 •

edited

Loading

fpdotmonkey Jul 9, 2024 •

edited

Loading

fpdotmonkey Jul 9, 2024

Bromeon Jul 9, 2024 •

edited

Loading

fpdotmonkey Jul 9, 2024

Bromeon Jul 8, 2024

fpdotmonkey Jul 9, 2024

Bromeon Jul 8, 2024

fpdotmonkey Jul 9, 2024

Bromeon Jul 9, 2024

Bromeon commented Jul 9, 2024

Support #[export(range = (radians_as_degrees, suffix="XX"))] #783

Support #[export(range = (radians_as_degrees, suffix="XX"))] #783

Conversation

fpdotmonkey commented Jun 27, 2024

GodotRust commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

fpdotmonkey Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

fpdotmonkey Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon Jul 9, 2024 • 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

Bromeon commented Jul 9, 2024

Support `#[export(range = (radians_as_degrees, suffix="XX"))]` #783

Support `#[export(range = (radians_as_degrees, suffix="XX"))]` #783

fpdotmonkey Jul 9, 2024 •

edited

Loading

fpdotmonkey Jul 9, 2024 •

edited

Loading

Bromeon Jul 9, 2024 •

edited

Loading