Merge pull request #487 from godot-rust/qol/revert-get-prefixes

Reintroduce `get_` prefixes (revert #477)

Author: Bromeon

.github/workflows/update-docs.yml

@@ -58,21 +58,25 @@ jobs:
       # Opened/reopened/updated PR: include PR author + title
       - name: "Construct JSON (for PR sync)"
         if: github.event_name == 'pull_request' && github.event.action != 'closed'
-        # Escape double quotes in PR title, as it will be used in a JSON string.
+        # jq escapes backticks, double quotes etc. in PR title.
         run: |
-          escapedPrTitle=$(echo "${{ github.event.pull_request.title }}" | sed 's/"/\\"/g')
-          payload=$(cat <<'HEREDOC'
-          {
+          payload=$(jq -n \
+          --arg repoOwner '${{ github.repository_owner }}' \
+          --arg num '${{ github.event.number }}' \
+          --arg commitSha '${{ github.event.pull_request.head.sha }}' \
+          --arg date '${{ github.event.pull_request.updated_at }}' \
+          --arg prAuthor '${{ github.event.pull_request.user.login }}' \
+          --arg prTitle '${{ github.event.pull_request.title }}' \
+          '{
             "op": "put",
             "repo": "gdext",
-            "repo-owner": "${{ github.repository_owner }}",
-            "num": "${{ github.event.number }}",
-            "commit-sha": "${{ github.event.pull_request.head.sha }}",
-            "date": "${{ github.event.pull_request.updated_at }}",
-            "pr-author": "${{ github.event.pull_request.user.login }}",
-            "pr-title": "$escapedPrTitle"
-          }
-          HEREDOC)
+            "repo-owner": $repoOwner,
+            "num": $num,
+            "commit-sha": $commitSha,
+            "date": $date,
+            "pr-author": $prAuthor,
+            "pr-title": $prTitle
+          }')
           echo "VAR=$payload"
           echo "GDEXT_JSON<<HEREDOC" >> $GITHUB_ENV
           echo "${payload}" >> $GITHUB_ENV

examples/dodge-the-creeps/rust/src/main_scene.rs

@@ -45,7 +45,7 @@ impl Main {
 
         self.score = 0;
 
-        player.bind_mut().start(start_position.position());
+        player.bind_mut().start(start_position.get_position());
         start_timer.start();
 
         let mut hud = self.base.get_node_as::<Hud>("Hud");
@@ -84,9 +84,9 @@ impl Main {
         let progress = rng.gen_range(u32::MIN..u32::MAX);
 
         mob_spawn_location.set_progress(progress as f32);
-        mob_scene.set_position(mob_spawn_location.position());
+        mob_scene.set_position(mob_spawn_location.get_position());
 
-        let mut direction = mob_spawn_location.rotation() + PI / 2.0;
+        let mut direction = mob_spawn_location.get_rotation() + PI / 2.0;
         direction += rng.gen_range(-PI / 4.0..PI / 4.0);
 
         mob_scene.set_rotation(direction);
@@ -101,7 +101,7 @@ impl Main {
         };
 
         mob.set_linear_velocity(Vector2::new(range, 0.0));
-        let lin_vel = mob.linear_velocity().rotated(real::from_f32(direction));
+        let lin_vel = mob.get_linear_velocity().rotated(real::from_f32(direction));
         mob.set_linear_velocity(lin_vel);
 
         let mut hud = self.base.get_node_as::<Hud>("Hud");

examples/dodge-the-creeps/rust/src/mob.rs

@@ -41,7 +41,7 @@ impl IRigidBody2D for Mob {
             .get_node_as::<AnimatedSprite2D>("AnimatedSprite2D");
 
         sprite.play();
-        let anim_names = sprite.sprite_frames().unwrap().animation_names();
+        let anim_names = sprite.get_sprite_frames().unwrap().get_animation_names();
 
         // TODO use pick_random() once implemented
         let anim_names = anim_names.to_vec();

examples/dodge-the-creeps/rust/src/player.rs

@@ -52,7 +52,7 @@ impl IArea2D for Player {
     }
 
     fn ready(&mut self) {
-        let viewport = self.base.viewport_rect();
+        let viewport = self.base.get_viewport_rect();
         self.screen_size = viewport.size;
         self.base.hide();
     }
@@ -101,7 +101,7 @@ impl IArea2D for Player {
         }
 
         let change = velocity * real::from_f64(delta);
-        let position = self.base.global_position() + change;
+        let position = self.base.get_global_position() + change;
         let position = Vector2::new(
             position.x.clamp(0.0, self.screen_size.x),
             position.y.clamp(0.0, self.screen_size.y),

godot-codegen/src/api_parser.rs

@@ -200,15 +200,6 @@ pub struct ClassMethod {
     pub arguments: Option<Vec<MethodArg>>,
 }
 
-impl ClassMethod {
-    pub fn map_args<R>(&self, f: impl FnOnce(&Vec<MethodArg>) -> R) -> R {
-        match self.arguments.as_ref() {
-            Some(args) => f(args),
-            None => f(&vec![]),
-        }
-    }
-}
-
 // Example: set_point_weight_scale ->
 // [ {name: "id", type: "int", meta: "int64"},
 //   {name: "weight_scale", type: "float", meta: "float"},

godot-codegen/src/class_generator.rs

@@ -8,7 +8,6 @@
 
 use proc_macro2::{Ident, TokenStream};
 use quote::{format_ident, quote};
-use std::borrow::Cow;
 use std::path::Path;
 
 use crate::api_parser::*;
@@ -1129,31 +1128,28 @@ fn make_class_method_definition(
 
     let class_name_str = &class_name.godot_ty;
     let godot_method_name = &method.name;
-    let renamed_method_name = special_cases::maybe_renamed(class_name, godot_method_name);
+    let rust_method_name = special_cases::maybe_renamed(class_name, godot_method_name);
 
-    let mut rust_method_name = Cow::Borrowed(renamed_method_name);
+    // Override const-qualification for known special cases (FileAccess::get_16, StreamPeer::get_u16, etc.).
+    /* TODO enable this once JSON/domain models are separated. Remove #[allow] above.
     let mut override_is_const = None;
-
-    if method.map_args(|args| args.is_empty()) {
-        // Getters (i.e. 0 arguments) are stripped of their `get_` prefix, to conform to Rust convention.
-        // Currently also applies to static methods, but NOT to methods which have default parameters but can be called with 0 arguments.
-        // TODO(bromeon): should we add #[doc(alias)]?
-        if let Some(remainder) = renamed_method_name.strip_prefix("get_") {
-            // Do not apply for FileAccess::get_16, StreamPeer::get_u16, etc.
-            if !special_cases::keeps_get_prefix(class_name, method) {
-                rust_method_name = Cow::Owned(remainder.to_string());
-
-                // Many getters are mutably qualified (GltfAccessor::get_max, CameraAttributes::get_exposure_multiplier, ...).
-                override_is_const = Some(true);
-            }
-        }
+    if let Some(override_const) = special_cases::is_method_const(class_name, &method) {
+        override_is_const = Some(override_const);
     }
 
-    let rust_method_name = rust_method_name.as_ref();
+    // Getters in particular are re-qualified as const (if there isn't already an override).
+    if override_is_const.is_none() && option_as_slice(&method.arguments).is_empty() {
+        if rust_method_name.starts_with("get_") {
+            // Many getters are mutably qualified (GltfAccessor::get_max, CameraAttributes::get_exposure_multiplier, ...).
+            // As a default, set those to const.
+            override_is_const = Some(true);
+        }
+    }*/
 
     let receiver = make_receiver(
         method.is_static,
-        override_is_const.unwrap_or(method.is_const),
+        //override_is_const.unwrap_or(method.is_const),
+        method.is_const,
         quote! { self.object_ptr },
     );
 
@@ -1912,8 +1908,8 @@ fn make_all_virtual_methods(
         all_virtuals.extend(
             get_methods_in_class(class)
                 .iter()
-                .cloned()
-                .filter(|m| m.is_virtual),
+                .filter(|m| m.is_virtual)
+                .cloned(),
         );
     };
 

godot-codegen/src/special_cases.rs

@@ -176,66 +176,36 @@ pub(crate) fn is_excluded_from_default_params(class_name: Option<&TyName>, godot
     }
 }
 
+/// Return `true` if a method should have `&self` receiver in Rust, `false` if `&mut self` and `None` if original qualifier should be kept.
+///
+/// In cases where the method falls under some general category (like getters) that have their own const-qualification overrides, `Some`
+/// should be returned to take precedence over general rules. Example: `FileAccess::get_pascal_string()` is mut, but would be const-qualified
+/// since it looks like a getter.
 #[rustfmt::skip]
-pub(crate) fn keeps_get_prefix(class_name: &TyName, method: &ClassMethod) -> bool {
-    // Also list those which have default parameters and can be called with 0 arguments. Those are anyway
-    // excluded at the moment, but this is more robust if the outer logic changes.
-
-    match (class_name.godot_ty.as_str(), method.name.as_str()) {
-        // For Object
-        // https://docs.godotengine.org/en/stable/classes/class_object.html#methods
-        | ("Object", "get_class")
-        | ("Object", "get_instance_id") // currently removed, but would be shadowed by Gd::instance_id().
-        | ("Object", "get_script")
-        | ("Object", "get_script_instance")
-        // The following ones often have slight variations with parameters, so it's more consistent to have get_signal_list() and
-        // get_signal_connection_list(signal). This may change in the future.
-        | ("Object", "get_incoming_connections")
-        | ("Object", "get_meta_list")
-        | ("Object", "get_method_list")
-        | ("Object", "get_property_list")
-        | ("Object", "get_signal_list")
-
-        // For Node
-        // https://docs.godotengine.org/en/stable/classes/class_node.html#methods
-        // TODO get_child_count?
-
-        // https://docs.godotengine.org/en/stable/classes/class_fileaccess.html#methods
+#[cfg(FALSE)] // TODO enable this once JSON/domain models are separated.
+pub(crate) fn is_method_const(class_name: &TyName, godot_method: &ClassMethod) -> Option<bool> {
+    match (class_name.godot_ty.as_str(), godot_method.name.as_str()) {
+        // Changed to mut.
         | ("FileAccess", "get_16")
         | ("FileAccess", "get_32")
         | ("FileAccess", "get_64")
         | ("FileAccess", "get_8")
-        | ("FileAccess", "get_as_text")
         | ("FileAccess", "get_csv_line")
-        | ("FileAccess", "get_double")
-        | ("FileAccess", "get_error") // If this has side effects, should definitely keep prefix. Not clear.
-        | ("FileAccess", "get_float")
-        | ("FileAccess", "get_line")
-        | ("FileAccess", "get_open_error")
-        | ("FileAccess", "get_pascal_string")
         | ("FileAccess", "get_real")
+        | ("FileAccess", "get_float")
+        | ("FileAccess", "get_double")
         | ("FileAccess", "get_var")
-
-        // https://docs.godotengine.org/en/stable/classes/class_streampeer.html#methods
-        // do for 8,16,32,64 and u*
+        | ("FileAccess", "get_line")
+        | ("FileAccess", "get_pascal_string") // already mut.
+        | ("StreamPeer", "get_8")
         | ("StreamPeer", "get_16")
         | ("StreamPeer", "get_32")
         | ("StreamPeer", "get_64")
-        | ("StreamPeer", "get_8")
-        | ("StreamPeer", "get_double")
         | ("StreamPeer", "get_float")
-        | ("StreamPeer", "get_string")
-        | ("StreamPeer", "get_u16")
-        | ("StreamPeer", "get_u32")
-        | ("StreamPeer", "get_u64")
-        | ("StreamPeer", "get_u8")
-        | ("StreamPeer", "get_utf8_string")
-        | ("StreamPeer", "get_var")
-
-        // Others that conflict with a verb:
-        | ("AnimationPlayer", "get_queue")
+        | ("StreamPeer", "get_double")
+        => Some(false),
 
-        => true, _ => false,
+        _ => None,
     }
 }
 

godot-core/src/obj/traits.rs

@@ -106,7 +106,7 @@ pub trait Share {
 ///     T: Inherits<Node>,
 /// {
 ///     let up = node.upcast(); // type Gd<Node> inferred
-///     println!("Node #{} with name {}", up.instance_id(), up.name());
+///     println!("Node #{} with name {}", up.instance_id(), up.get_name());
 ///     up.free();
 /// }
 ///

itest/godot/SpecialTests.gd

@@ -33,7 +33,7 @@ func test_collision_object_2d_input_event():
 	window.add_child(collision_object)
 
 	assert_that(not collision_object.input_event_called())
-	assert_eq(collision_object.viewport(), null)
+	assert_eq(collision_object.get_viewport(), null)
 
 	var event := InputEventMouseMotion.new()
 	event.global_position = Vector2.ZERO
@@ -43,7 +43,7 @@ func test_collision_object_2d_input_event():
 	await root.get_tree().physics_frame
 
 	assert_that(collision_object.input_event_called())
-	assert_eq(collision_object.viewport(), window)
+	assert_eq(collision_object.get_viewport(), window)
 
 	window.queue_free()
 

itest/rust/src/builtin_tests/containers/array_test.rs

@@ -398,7 +398,7 @@ fn typed_array_pass_to_godot_func() {
     let error = texture.create_from_images(images);
 
     assert_eq!(error, Error::OK);
-    assert_eq!((texture.width(), texture.height()), (2, 4));
+    assert_eq!((texture.get_width(), texture.get_height()), (2, 4));
 }
 
 #[itest]