From 93bc73d1b9f881b501f18d86dea10f013973849e Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Thu, 23 Mar 2023 00:14:06 +0100 Subject: [PATCH 1/6] Test runner: print test name before execution --- godot-core/src/lib.rs | 9 +++++++++ itest/godot/TestSuite.gd | 12 +++++++---- itest/rust/src/runner.rs | 43 +++++++++++++++++++++++++--------------- 3 files changed, 44 insertions(+), 20 deletions(-) diff --git a/godot-core/src/lib.rs b/godot-core/src/lib.rs index bfd7fdbd7..80144d856 100644 --- a/godot-core/src/lib.rs +++ b/godot-core/src/lib.rs @@ -69,12 +69,21 @@ pub mod private { match std::panic::catch_unwind(code) { Ok(result) => Some(result), Err(err) => { + // Flush, to make sure previous Rust output (e.g. test announcement, or debug prints during app) have been printed + // TODO write custom panic handler and move this there, before panic backtrace printing + flush_stdout(); + log::godot_error!("Rust function panicked. Context: {}", error_context()); print_panic(err); None } } } + + pub fn flush_stdout() { + use std::io::Write; + std::io::stdout().flush().expect("flush stdout"); + } } #[cfg(feature = "trace")] diff --git a/itest/godot/TestSuite.gd b/itest/godot/TestSuite.gd index bde7b4b3c..b766a5fca 100644 --- a/itest/godot/TestSuite.gd +++ b/itest/godot/TestSuite.gd @@ -14,10 +14,12 @@ func assert_that(what: bool, message: String = "") -> bool: return true _assertion_failed = true + + printerr() # previous line not yet broken if message: - print("assertion failed: %s" % message) + push_error("GDScript assertion failed: %s" % message) else: - print("assertion failed") + push_error("GDScript assertion failed.") return false func assert_eq(left, right, message: String = "") -> bool: @@ -25,8 +27,10 @@ func assert_eq(left, right, message: String = "") -> bool: return true _assertion_failed = true + + printerr() # previous line not yet broken if message: - print("assertion failed: %s\n left: %s\n right: %s" % [message, left, right]) + push_error("GDScript assertion failed: %s\n left: %s\n right: %s" % [message, left, right]) else: - print("assertion failed: `(left == right)`\n left: %s\n right: %s" % [left, right]) + push_error("GDScript assertion failed: `(left == right)`\n left: %s\n right: %s" % [left, right]) return false diff --git a/itest/rust/src/runner.rs b/itest/rust/src/runner.rs index b072d5e98..70c452f73 100644 --- a/itest/rust/src/runner.rs +++ b/itest/rust/src/runner.rs @@ -71,27 +71,29 @@ impl IntegrationTests { let mut last_file = None; for test in tests { + print_test_pre(test.name, test.file.to_string(), &mut last_file, false); let outcome = run_rust_test(&test, &ctx); self.update_stats(&outcome); - print_test(test.file.to_string(), test.name, outcome, &mut last_file); + print_test_post(test.name, outcome); } } fn run_gdscript_tests(&mut self, tests: VariantArray) { let mut last_file = None; for test in tests.iter_shared() { + let test_file = get_property(&test, "suite_name"); + let test_case = get_property(&test, "method_name"); + + print_test_pre(&test_case, test_file, &mut last_file, true); let result = test.call("run", &[]); let success = result.try_to::().unwrap_or_else(|_| { panic!("GDScript test case {test} returned non-bool: {result}") }); - - let test_file = get_property(&test, "suite_name"); - let test_case = get_property(&test, "method_name"); let outcome = TestOutcome::from_bool(success); self.update_stats(&outcome); - print_test(test_file, &test_case, outcome, &mut last_file); + print_test_post(&test_case, outcome); } } @@ -178,16 +180,7 @@ fn run_rust_test(test: &RustTestCase, ctx: &TestContext) -> TestOutcome { TestOutcome::from_bool(success.is_some()) } -/// Prints a test name and its outcome. -/// -/// Note that this is run after a test run, so stdout/stderr output during the test will be printed before. -/// It would be possible to print the test name before and the outcome after, but that would split or duplicate the line. -fn print_test( - test_file: String, - test_case: &str, - outcome: TestOutcome, - last_file: &mut Option, -) { +fn print_test_pre(test_case: &str, test_file: String, last_file: &mut Option, flush: bool) { // Check if we need to open a new category for a file let print_file = last_file .as_ref() @@ -203,12 +196,30 @@ fn print_test( println!("\n {file_subtitle}:"); } - println!(" -- {test_case} ... {outcome}"); + print!(" -- {test_case} ... "); + if flush { + // Flush in GDScript, because its own print may come sooner than Rust prints otherwise + // (strictly speaking, this can also happen from Rust, when Godot prints something. So far, it didn't though... + godot::private::flush_stdout(); + } // State update for file-category-print *last_file = Some(test_file); } +/// Prints a test name and its outcome. +/// +/// Note that this is run after a test run, so stdout/stderr output during the test will be printed before. +/// It would be possible to print the test name before and the outcome after, but that would split or duplicate the line. +fn print_test_post(test_case: &str, outcome: TestOutcome) { + // If test failed, something was printed (e.g. assertion), so we can print the entire line again; otherwise just outcome on same line. + if matches!(outcome, TestOutcome::Failed) { + println!(" -- {test_case} ... {outcome}"); + } else { + println!("{outcome}"); + } +} + fn get_property(test: &Variant, property: &str) -> String { test.call("get", &[property.to_variant()]).to::() } From d6d73e4980d5df1429628d94e20a1705bec3840c Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Thu, 23 Mar 2023 00:14:50 +0100 Subject: [PATCH 2/6] More generated `#[func]` tests (argument/return value passing between GDScript and Rust) --- godot-core/src/builtin/array.rs | 15 ++++++- godot-core/src/builtin/meta/mod.rs | 45 +++++++------------- godot-core/src/builtin/node_path.rs | 2 + godot-core/src/builtin/variant/impls.rs | 18 +++++++- godot-macros/src/derive_godot_class.rs | 16 +++---- itest/godot/input/GenFfiTests.template.gd | 27 +++++++++--- itest/rust/build.rs | 51 ++++++++++++++++++----- 7 files changed, 118 insertions(+), 56 deletions(-) diff --git a/godot-core/src/builtin/array.rs b/godot-core/src/builtin/array.rs index d13ce4566..61f09c0d1 100644 --- a/godot-core/src/builtin/array.rs +++ b/godot-core/src/builtin/array.rs @@ -855,7 +855,7 @@ macro_rules! varray { /// [`set_typed`](https://docs.godotengine.org/en/latest/classes/class_array.html#class-array-method-set-typed). /// /// We ignore the `script` parameter because it has no impact on typing in Godot. -#[derive(Debug, PartialEq, Eq)] +#[derive(PartialEq, Eq)] struct TypeInfo { variant_type: VariantType, class_name: StringName, @@ -880,3 +880,16 @@ impl TypeInfo { self.variant_type != VariantType::Nil } } + +impl fmt::Debug for TypeInfo { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let class = self.class_name.to_string(); + let class_str = if class.is_empty() { + String::new() + } else { + format!(" (class={class})") + }; + + write!(f, "{:?}{}", self.variant_type, class_str) + } +} diff --git a/godot-core/src/builtin/meta/mod.rs b/godot-core/src/builtin/meta/mod.rs index 9006a8b84..534d8d31a 100644 --- a/godot-core/src/builtin/meta/mod.rs +++ b/godot-core/src/builtin/meta/mod.rs @@ -27,13 +27,14 @@ pub trait VariantMetadata { } fn property_info(property_name: &str) -> PropertyInfo { - PropertyInfo::new( - Self::variant_type(), - Self::class_name(), - StringName::from(property_name), - global::PropertyHint::PROPERTY_HINT_NONE, - GodotString::new(), - ) + PropertyInfo { + variant_type: Self::variant_type(), + class_name: Self::class_name(), + property_name: StringName::from(property_name), + hint: global::PropertyHint::PROPERTY_HINT_NONE, + hint_string: GodotString::new(), + usage: global::PropertyUsageFlags::PROPERTY_USAGE_DEFAULT, + } } fn param_metadata() -> sys::GDExtensionClassMethodArgumentMetadata { @@ -52,33 +53,17 @@ impl VariantMetadata for Option { /// Rusty abstraction of sys::GDExtensionPropertyInfo /// Keeps the actual allocated values (the sys equivalent only keeps pointers, which fall out of scope) #[derive(Debug)] +// Note: is not #[non_exhaustive], so adding fields is a breaking change. Mostly used internally at the moment though. pub struct PropertyInfo { - variant_type: VariantType, - class_name: ClassName, - property_name: StringName, - hint: global::PropertyHint, - hint_string: GodotString, - usage: global::PropertyUsageFlags, + pub variant_type: VariantType, + pub class_name: ClassName, + pub property_name: StringName, + pub hint: global::PropertyHint, + pub hint_string: GodotString, + pub usage: global::PropertyUsageFlags, } impl PropertyInfo { - pub fn new( - variant_type: VariantType, - class_name: ClassName, - property_name: StringName, - hint: global::PropertyHint, - hint_string: GodotString, - ) -> Self { - Self { - variant_type, - class_name, - property_name, - hint, - hint_string, - usage: global::PropertyUsageFlags::PROPERTY_USAGE_DEFAULT, - } - } - /// Converts to the FFI type. Keep this object allocated while using that! pub fn property_sys(&self) -> sys::GDExtensionPropertyInfo { use crate::obj::EngineEnum as _; diff --git a/godot-core/src/builtin/node_path.rs b/godot-core/src/builtin/node_path.rs index 191bb13b5..942f33fe4 100644 --- a/godot-core/src/builtin/node_path.rs +++ b/godot-core/src/builtin/node_path.rs @@ -90,5 +90,7 @@ impl_builtin_traits! { Default => node_path_construct_default; Clone => node_path_construct_copy; Drop => node_path_destroy; + Eq => node_path_operator_equal; + // Ord => node_path_operator_less; } } diff --git a/godot-core/src/builtin/variant/impls.rs b/godot-core/src/builtin/variant/impls.rs index 1365a4cb9..cbe3d7727 100644 --- a/godot-core/src/builtin/variant/impls.rs +++ b/godot-core/src/builtin/variant/impls.rs @@ -5,8 +5,9 @@ */ use super::*; -use crate::builtin::meta::VariantMetadata; +use crate::builtin::meta::{PropertyInfo, VariantMetadata}; use crate::builtin::*; +use crate::engine::global; use crate::obj::EngineEnum; use godot_ffi as sys; use sys::GodotFfi; @@ -224,6 +225,21 @@ impl VariantMetadata for Variant { // Arrays use the `NIL` type to indicate that they are untyped. VariantType::Nil } + + fn property_info(property_name: &str) -> PropertyInfo { + PropertyInfo { + variant_type: Self::variant_type(), + class_name: Self::class_name(), + property_name: StringName::from(property_name), + hint: global::PropertyHint::PROPERTY_HINT_NONE, + hint_string: GodotString::new(), + usage: global::PropertyUsageFlags::PROPERTY_USAGE_NIL_IS_VARIANT, + } + } + + fn param_metadata() -> sys::GDExtensionClassMethodArgumentMetadata { + sys::GDEXTENSION_METHOD_ARGUMENT_METADATA_INT_IS_INT8 + } } impl ToVariant for T { diff --git a/godot-macros/src/derive_godot_class.rs b/godot-macros/src/derive_godot_class.rs index f05e83589..780bb97b8 100644 --- a/godot-macros/src/derive_godot_class.rs +++ b/godot-macros/src/derive_godot_class.rs @@ -379,14 +379,14 @@ fn make_exports_impl(class_name: &Ident, fields: &Fields) -> TokenStream { use ::godot::builtin::meta::VariantMetadata; let class_name = ::godot::builtin::StringName::from(#class_name::CLASS_NAME); - - let property_info = ::godot::builtin::meta::PropertyInfo::new( - <#field_type>::variant_type(), - ::godot::builtin::meta::ClassName::of::<#class_name>(), - ::godot::builtin::StringName::from(#field_name), - ::godot::engine::global::PropertyHint::#hint_type, - ::godot::builtin::GodotString::from(#description), - ); + let property_info = ::godot::builtin::meta::PropertyInfo { + variant_type: <#field_type>::variant_type(), + class_name: ::godot::builtin::meta::ClassName::of::<#class_name>(), + property_name: ::godot::builtin::StringName::from(#field_name), + hint: ::godot::engine::global::PropertyHint::#hint_type, + hint_string: ::godot::builtin::GodotString::from(#description), + usage: ::godot::engine::global::PropertyUsageFlags::PROPERTY_USAGE_DEFAULT, + }; let property_info_sys = property_info.property_sys(); let getter_name = ::godot::builtin::StringName::from(#getter_name); diff --git a/itest/godot/input/GenFfiTests.template.gd b/itest/godot/input/GenFfiTests.template.gd index 91ee508f9..6a820b089 100644 --- a/itest/godot/input/GenFfiTests.template.gd +++ b/itest/godot/input/GenFfiTests.template.gd @@ -4,14 +4,31 @@ extends TestSuite +# Note: GDScript only uses ptrcalls if it has the full type information available at "compile" (parse) time. +# That includes all arguments (including receiver) as well as function signature (parameters + return type). +# Otherwise, GDScript will use varcall. Both are tested below. +# It is thus important that `ffi` is initialized using = for varcalls, and using := for ptrcalls. + #( func test_varcall_IDENT(): var ffi = GenFfi.new() - var from_rust = ffi.return_IDENT() - assert_that(ffi.accept_IDENT(from_rust)) + var from_rust: Variant = ffi.return_IDENT() + assert_that(ffi.accept_IDENT(from_rust), "ffi.accept_IDENT(from_rust)") + + var from_gdscript: Variant = VAL + var mirrored: Variant = ffi.mirror_IDENT(from_gdscript) + assert_eq(mirrored, from_gdscript, "mirrored == from_gdscript") +#) + +#( +func test_ptrcall_IDENT(): + var ffi := GenFfi.new() + + var from_rust: TYPE = ffi.return_IDENT() + assert_that(ffi.accept_IDENT(from_rust), "ffi.accept_IDENT(from_rust)") - var from_gdscript = VAL - var mirrored = ffi.mirror_IDENT(from_gdscript) - assert_eq(mirrored, from_gdscript) + var from_gdscript: TYPE = VAL + var mirrored: TYPE = ffi.mirror_IDENT(from_gdscript) + assert_eq(mirrored, from_gdscript, "mirrored == from_gdscript") #) diff --git a/itest/rust/build.rs b/itest/rust/build.rs index ec3ca4708..e6bfe1241 100644 --- a/itest/rust/build.rs +++ b/itest/rust/build.rs @@ -11,22 +11,33 @@ use std::path::Path; type IoResult = std::io::Result<()>; -macro_rules! push { - ($inputs:ident; $gdscript_ty:ident, $rust_ty:ty, $val:expr) => { - push!($inputs; $gdscript_ty, $rust_ty, $val, $val); - }; - - ($inputs:ident; $gdscript_ty:ident, $rust_ty:ty, $gdscript_val:expr, $rust_val:expr) => { +/// Push with GDScript expr in string +macro_rules! pushs { + ($inputs:ident; $GDScriptTy:expr, $RustTy:ty, $gdscript_val:expr, $rust_val:expr) => { $inputs.push(Input { - ident: stringify!($rust_ty).to_ascii_lowercase(), - gdscript_ty: stringify!($gdscript_ty), - gdscript_val: stringify!($gdscript_val), - rust_ty: quote! { $rust_ty }, + ident: stringify!($RustTy) + .to_ascii_lowercase() + .replace("<", "_") + .replace(">", ""), + gdscript_ty: stringify!($GDScriptTy), + gdscript_val: $gdscript_val, + rust_ty: quote! { $RustTy }, rust_val: quote! { $rust_val }, }); }; } +/// Push simple GDScript expression, outside string +macro_rules! push { + ($inputs:ident; $GDScriptTy:expr, $RustTy:ty, $val:expr) => { + push!($inputs; $GDScriptTy, $RustTy, $val, $val); + }; + + ($inputs:ident; $GDScriptTy:expr, $RustTy:ty, $gdscript_val:expr, $rust_val:expr) => { + pushs!($inputs; $GDScriptTy, $RustTy, stringify!($gdscript_val), $rust_val); + }; +} + // Edit this to change involved types fn collect_inputs() -> Vec { let mut inputs = vec![]; @@ -41,15 +52,33 @@ fn collect_inputs() -> Vec { push!(inputs; bool, bool, true); push!(inputs; Color, Color, Color(0.7, 0.5, 0.3, 0.2), Color::from_rgba(0.7, 0.5, 0.3, 0.2)); push!(inputs; String, GodotString, "hello", "hello".into()); + push!(inputs; StringName, StringName, &"hello", "hello".into()); + pushs!(inputs; NodePath, NodePath, r#"^"hello""#, "hello".into()); push!(inputs; Vector2, Vector2, Vector2(12.5, -3.5), Vector2::new(12.5, -3.5)); push!(inputs; Vector3, Vector3, Vector3(117.5, 100.0, -323.25), Vector3::new(117.5, 100.0, -323.25)); push!(inputs; Vector4, Vector4, Vector4(-18.5, 24.75, -1.25, 777.875), Vector4::new(-18.5, 24.75, -1.25, 777.875)); push!(inputs; Vector2i, Vector2i, Vector2i(-2147483648, 2147483647), Vector2i::new(-2147483648, 2147483647)); push!(inputs; Vector3i, Vector3i, Vector3i(-1, -2147483648, 2147483647), Vector3i::new(-1, -2147483648, 2147483647)); - //push!(inputs; Variant, Variant, 123, 123i64.to_variant()); + + // Data structures + // TODO enable below, when GDScript has typed array literals, or find a hack with eval/lambdas + /*pushs!(inputs; Array[int], Array, + "(func() -> Array[int]: [-7, 12, 40])()", + array![-7, 12, 40] + );*/ + + push!(inputs; Array, VariantArray, + [-7, "godot", false, Vector2i(-77, 88)], + varray![-7, "godot", false, Vector2i::new(-77, 88)]); + + pushs!(inputs; Dictionary, Dictionary, + r#"{"key": 83, -3: Vector2(1, 2), 0.03: true}"#, + dict! { "key": 83, (-3): Vector2::new(1.0, 2.0), 0.03: true } + ); // Composite push!(inputs; int, InstanceId, -1, InstanceId::from_nonzero(0xFFFFFFFFFFFFFFF)); + push!(inputs; Variant, Variant, 123, 123i64.to_variant()); inputs } From 3073f3a5dfd347b3f84fde0b204419275986a3d1 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Thu, 23 Mar 2023 19:36:29 +0100 Subject: [PATCH 3/6] Projection tests: fix is_equal_approx bug, reduce number of iterations --- itest/rust/src/projection_test.rs | 155 ++++++++++++++++-------------- 1 file changed, 82 insertions(+), 73 deletions(-) diff --git a/itest/rust/src/projection_test.rs b/itest/rust/src/projection_test.rs index 3641ad735..5d3bd2b45 100644 --- a/itest/rust/src/projection_test.rs +++ b/itest/rust/src/projection_test.rs @@ -15,9 +15,9 @@ fn matrix_eq_approx(a: Projection, b: Projection) -> bool { let v1 = a.cols[i]; let v2 = b.cols[i]; if !is_equal_approx(v1.x, v2.x) - && !is_equal_approx(v1.y, v2.y) - && !is_equal_approx(v1.z, v2.z) - && !is_equal_approx(v1.w, v2.w) + || !is_equal_approx(v1.y, v2.y) + || !is_equal_approx(v1.z, v2.z) + || !is_equal_approx(v1.w, v2.w) { return false; } @@ -39,12 +39,12 @@ fn test_create_orthogonal() { for [left, right, bottom, top, near, far] in TEST_DATA { let rust_proj = Projection::create_orthogonal(left, right, bottom, top, near, far); let godot_proj = InnerProjection::create_orthogonal( - left as _, - right as _, - bottom as _, - top as _, - near as _, - far as _, + left.as_f64(), + right.as_f64(), + bottom.as_f64(), + top.as_f64(), + near.as_f64(), + far.as_f64(), ); assert_eq_approx!( @@ -70,10 +70,10 @@ fn test_create_orthogonal_aspect() { for (size, aspect, near, far, flip_fov) in TEST_DATA { let rust_proj = Projection::create_orthogonal_aspect(size, aspect, near, far, flip_fov); let godot_proj = InnerProjection::create_orthogonal_aspect( - size as _, - aspect as _, - near as _, - far as _, + size.as_f64(), + aspect.as_f64(), + near.as_f64(), + far.as_f64(), flip_fov, ); @@ -99,10 +99,10 @@ fn test_create_perspective() { for (fov_y, aspect, near, far, flip_fov) in TEST_DATA { let rust_proj = Projection::create_perspective(fov_y, aspect, near, far, flip_fov); let godot_proj = InnerProjection::create_perspective( - fov_y as _, - aspect as _, - near as _, - far as _, + fov_y.as_f64(), + aspect.as_f64(), + near.as_f64(), + far.as_f64(), flip_fov, ); @@ -126,12 +126,12 @@ fn test_create_frustum() { for [left, right, bottom, top, near, far] in TEST_DATA { let rust_proj = Projection::create_frustum(left, right, bottom, top, near, far); let godot_proj = InnerProjection::create_frustum( - left as _, - right as _, - bottom as _, - top as _, - near as _, - far as _, + left.as_f64(), + right.as_f64(), + bottom.as_f64(), + top.as_f64(), + near.as_f64(), + far.as_f64(), ); assert_eq_approx!( @@ -155,12 +155,13 @@ fn test_create_frustum_aspect() { for (size, aspect, offset, near, far, flip_fov) in TEST_DATA { let rust_proj = Projection::create_frustum_aspect(size, aspect, offset, near, far, flip_fov); + let godot_proj = InnerProjection::create_frustum_aspect( - size as _, - aspect as _, + size.as_f64(), + aspect.as_f64(), offset, - near as _, - far as _, + near.as_f64(), + far.as_f64(), flip_fov, ); @@ -177,28 +178,32 @@ fn test_create_frustum_aspect() { #[itest] fn test_projection_combined() { + let range = [0, 5, 10, 15, 20]; + fn f(v: isize) -> real { (v as real) * 0.5 - 0.5 } + // Orthogonal - for left_i in 0..20 { + for left_i in range { let left = f(left_i); - for right in (left_i + 1..=20).map(f) { - for bottom_i in 0..20 { + for right in range.map(|v| f(v + left_i)) { + for bottom_i in range { let bottom = f(bottom_i); - for top in (bottom_i + 1..=20).map(f) { - for near_i in 0..20 { + for top in range.map(|v| f(v + bottom_i)) { + for near_i in range { let near = f(near_i); - for far in (near_i + 1..=20).map(f) { + for far in range.map(|v| f(v + near_i)) { let rust_proj = Projection::create_orthogonal(left, right, bottom, top, near, far); + let godot_proj = InnerProjection::create_orthogonal( - left as _, - right as _, - bottom as _, - top as _, - near as _, - far as _, + left.as_f64(), + right.as_f64(), + bottom.as_f64(), + top.as_f64(), + near.as_f64(), + far.as_f64(), ); assert_eq_approx!( @@ -220,20 +225,21 @@ fn test_projection_combined() { } // Perspective - for fov_y in (0..18).map(|v| (v as real) * 10.0) { - for aspect_x in 1..=10 { - for aspect_y in 1..=10 { + for fov_y in [3, 6, 12, 15].map(|v| (v as real) * 10.0) { + for aspect_x in 1..=3 { + for aspect_y in 1..=3 { let aspect = (aspect_x as real) / (aspect_y as real); - for near_i in 1..10 { + for near_i in 1..4 { let near = near_i as real; - for far in (near_i + 1..=20).map(|v| v as real) { + for far in range.map(|v| (v + near_i + 1) as real) { let rust_proj = Projection::create_perspective(fov_y, aspect, near, far, false); + let godot_proj = InnerProjection::create_perspective( - fov_y as _, - aspect as _, - near as _, - far as _, + fov_y.as_f64(), + aspect.as_f64(), + near.as_f64(), + far.as_f64(), false, ); @@ -255,24 +261,25 @@ fn test_projection_combined() { } // Frustum - for left_i in 0..20 { + for left_i in range { let left = f(left_i); - for right in (left_i + 1..=20).map(f) { - for bottom_i in 0..20 { + for right in range.map(|v| f(v + left_i + 1)) { + for bottom_i in range { let bottom = f(bottom_i); - for top in (bottom_i + 1..=20).map(f) { - for near_i in 0..20 { + for top in range.map(|v| f(v + bottom_i + 1)) { + for near_i in range { let near = (near_i as real) * 0.5; - for far in (near_i + 1..=20).map(|v| (v as real) * 0.5) { + for far in range.map(|v| ((v + near_i + 1) as real) * 0.5) { let rust_proj = Projection::create_frustum(left, right, bottom, top, near, far); + let godot_proj = InnerProjection::create_frustum( - left as _, - right as _, - bottom as _, - top as _, - near as _, - far as _, + left.as_f64(), + right.as_f64(), + bottom.as_f64(), + top.as_f64(), + near.as_f64(), + far.as_f64(), ); assert_eq_approx!( @@ -294,13 +301,14 @@ fn test_projection_combined() { } // Size, Aspect, Near, Far - for size in (1..=10).map(|v| v as real) { - for aspect_x in 1..=10 { - for aspect_y in 1..=10 { + let range = [1, 4, 7, 10]; + for size in range.map(|v| v as real) { + for aspect_x in range { + for aspect_y in range { let aspect = (aspect_x as real) / (aspect_y as real); - for near_i in 1..10 { + for near_i in range { let near = near_i as real; - for far in (near_i + 1..=20).map(|v| v as real) { + for far in range.map(|v| (v + near_i) as real) { let rust_proj_frustum = Projection::create_frustum_aspect( size, aspect, @@ -310,11 +318,11 @@ fn test_projection_combined() { false, ); let godot_proj_frustum = InnerProjection::create_frustum_aspect( - size as _, - aspect as _, + size.as_f64(), + aspect.as_f64(), Vector2::ZERO, - near as _, - far as _, + near.as_f64(), + far.as_f64(), false, ); @@ -327,11 +335,12 @@ fn test_projection_combined() { let rust_proj_ortho = Projection::create_orthogonal_aspect(size, aspect, near, far, false); + let godot_proj_ortho = InnerProjection::create_orthogonal_aspect( - size as _, - aspect as _, - near as _, - far as _, + size.as_f64(), + aspect.as_f64(), + near.as_f64(), + far.as_f64(), false, ); From 3d0e3d2bc0dd15f1270e14dd0fa538d98d33f632 Mon Sep 17 00:00:00 2001 From: thebigG Date: Wed, 22 Mar 2023 08:26:08 -0500 Subject: [PATCH 4/6] -Fixes godot-rust/gdext#165. (cherry picked from commit 4906beab43e2d3584e3f7916611f5272a6584bdc) --- godot-core/src/builtin/meta/signature.rs | 5 +++-- itest/godot/InheritTests.gd | 27 ++++++++++++++++++++++++ itest/godot/TestRunner.gd | 1 + 3 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 itest/godot/InheritTests.gd diff --git a/godot-core/src/builtin/meta/signature.rs b/godot-core/src/builtin/meta/signature.rs index 415f8f1d4..cce236dfe 100644 --- a/godot-core/src/builtin/meta/signature.rs +++ b/godot-core/src/builtin/meta/signature.rs @@ -162,8 +162,9 @@ macro_rules! impl_signature_for_tuple { unsafe { <$R as sys::GodotFuncMarshal>::try_write_sys(&ret_val, ret) } .unwrap_or_else(|e| return_error::<$R>(method_name, &e)); - // FIXME is inc_ref needed here? - // std::mem::forget(ret_val); + //Note: we want to prevent the destructor from being run, since we pass ownership to the caller. + //https://github.com/godot-rust/gdext/pull/165 + std::mem::forget(ret_val); } } }; diff --git a/itest/godot/InheritTests.gd b/itest/godot/InheritTests.gd new file mode 100644 index 000000000..d89704509 --- /dev/null +++ b/itest/godot/InheritTests.gd @@ -0,0 +1,27 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at https://mozilla.org/MPL/2.0/. + +extends ArrayTest + +var test_suite: TestSuite = TestSuite.new() + +# In order to reproduce the behavior discovered in https://github.com/godot-rust/gdext/issues/138 +# we must inherit a Godot Node. Because of this we can't just inherit TesSuite like the rest of the tests. +func assert_that(what: bool, message: String = "") -> bool: + return test_suite.assert_that(what, message) + +func assert_eq(left, right, message: String = "") -> bool: + return test_suite.assert_eq(left, right, message) + +# Called when the node enters the scene tree for the first time. +func _ready(): + pass + +func test_vector_array_return_from_user_func(): + var array: Array = return_typed_array(2) + assert_eq(array, [1,2]) + +# Called every frame. 'delta' is the elapsed time since the previous frame. +func _process(delta): + pass diff --git a/itest/godot/TestRunner.gd b/itest/godot/TestRunner.gd index 4dedc56c0..3e28abe2f 100644 --- a/itest/godot/TestRunner.gd +++ b/itest/godot/TestRunner.gd @@ -24,6 +24,7 @@ func _ready(): var gdscript_suites: Array = [ preload("res://ManualFfiTests.gd").new(), preload("res://gen/GenFfiTests.gd").new(), + preload("res://InheritTests.gd").new() ] var gdscript_tests: Array = [] From b5e74b3159f64d796196b799b94367f8b488d7f5 Mon Sep 17 00:00:00 2001 From: Lili Zoey Date: Sun, 26 Mar 2023 19:40:19 +0200 Subject: [PATCH 5/6] Fix various issues with pointer calls Make `Gd` in virtual method-calls use `ref_get_object`/`ref_set_object` if it inherits from `RefCounted` Add some tests of objects in pointer calls Add tests for Array passing through pointer calls Add more tests for refcounted Add a test for _input Rename some stuff in TestContext Add unsafe to rect and such GodotFfi impl along with `SAFETY` comments --- godot-codegen/src/central_generator.rs | 8 +- godot-codegen/src/lib.rs | 4 + godot-codegen/src/util.rs | 2 +- godot-core/src/builtin/aabb.rs | 4 +- godot-core/src/builtin/array.rs | 22 ++- godot-core/src/builtin/basis.rs | 4 +- godot-core/src/builtin/color.rs | 4 +- godot-core/src/builtin/dictionary.rs | 22 ++- godot-core/src/builtin/macros.rs | 5 +- godot-core/src/builtin/meta/signature.rs | 18 +- godot-core/src/builtin/node_path.rs | 22 ++- godot-core/src/builtin/packed_array.rs | 22 ++- godot-core/src/builtin/plane.rs | 4 +- godot-core/src/builtin/projection.rs | 4 +- godot-core/src/builtin/quaternion.rs | 4 +- godot-core/src/builtin/rect2.rs | 4 +- godot-core/src/builtin/rect2i.rs | 4 +- godot-core/src/builtin/rid.rs | 4 +- godot-core/src/builtin/string.rs | 31 +++- godot-core/src/builtin/string_name.rs | 23 ++- godot-core/src/builtin/transform2d.rs | 4 +- godot-core/src/builtin/transform3d.rs | 4 +- godot-core/src/builtin/variant/mod.rs | 6 +- godot-core/src/builtin/vector2.rs | 8 +- godot-core/src/builtin/vector2i.rs | 8 +- godot-core/src/builtin/vector3.rs | 8 +- godot-core/src/builtin/vector3i.rs | 8 +- godot-core/src/builtin/vector4.rs | 8 +- godot-core/src/builtin/vector4i.rs | 8 +- godot-core/src/macros.rs | 35 ++-- godot-core/src/obj/gd.rs | 47 ++++- godot-core/src/obj/instance_id.rs | 4 +- godot-core/src/obj/traits.rs | 8 + godot-core/src/registry.rs | 5 +- godot-ffi/src/godot_ffi.rs | 208 ++++++++++++++++------- godot-ffi/src/lib.rs | 2 +- itest/godot/ManualFfiTests.gd | 102 ++++++++++- itest/godot/TestRunner.gd | 2 + itest/rust/build.rs | 3 + itest/rust/src/lib.rs | 7 +- itest/rust/src/node_test.rs | 4 +- itest/rust/src/object_test.rs | 72 +++++++- itest/rust/src/runner.rs | 23 ++- itest/rust/src/variant_test.rs | 7 +- itest/rust/src/virtual_methods_test.rs | 55 +++++- 45 files changed, 695 insertions(+), 166 deletions(-) diff --git a/godot-codegen/src/central_generator.rs b/godot-codegen/src/central_generator.rs index 2d0adafcb..b98383a03 100644 --- a/godot-codegen/src/central_generator.rs +++ b/godot-codegen/src/central_generator.rs @@ -176,7 +176,9 @@ fn make_sys_code(central_items: &CentralItems) -> String { } } - impl GodotFfi for VariantType { + // SAFETY: + // This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. + unsafe impl GodotFfi for VariantType { ffi_methods! { type GDExtensionTypePtr = *mut Self; .. } } @@ -205,7 +207,9 @@ fn make_sys_code(central_items: &CentralItems) -> String { } } - impl GodotFfi for VariantOperator { + // SAFETY: + // This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. + unsafe impl GodotFfi for VariantOperator { ffi_methods! { type GDExtensionTypePtr = *mut Self; .. } } }; diff --git a/godot-codegen/src/lib.rs b/godot-codegen/src/lib.rs index 37b2d07dd..470af9672 100644 --- a/godot-codegen/src/lib.rs +++ b/godot-codegen/src/lib.rs @@ -271,6 +271,8 @@ const SELECTED_CLASSES: &[&str] = &[ "Image", "ImageTextureLayered", "Input", + "InputEvent", + "InputEventAction", "Label", "MainLoop", "Marker2D", @@ -296,4 +298,6 @@ const SELECTED_CLASSES: &[&str] = &[ "TextureLayered", "Time", "Timer", + "Window", + "Viewport", ]; diff --git a/godot-codegen/src/util.rs b/godot-codegen/src/util.rs index 8a3c39d49..08c8bd18f 100644 --- a/godot-codegen/src/util.rs +++ b/godot-codegen/src/util.rs @@ -99,7 +99,7 @@ pub fn make_enum_definition(enum_: &Enum) -> TokenStream { self.ord } } - impl sys::GodotFfi for #enum_name { + unsafe impl sys::GodotFfi for #enum_name { sys::ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } #bitfield_ops diff --git a/godot-core/src/builtin/aabb.rs b/godot-core/src/builtin/aabb.rs index f5d9e58c3..e1d62767f 100644 --- a/godot-core/src/builtin/aabb.rs +++ b/godot-core/src/builtin/aabb.rs @@ -82,6 +82,8 @@ impl Aabb { */ } -impl GodotFfi for Aabb { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Aabb { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/array.rs b/godot-core/src/builtin/array.rs index 61f09c0d1..e1bfee88e 100644 --- a/godot-core/src/builtin/array.rs +++ b/godot-core/src/builtin/array.rs @@ -566,8 +566,26 @@ impl Array { // ... // } -impl GodotFfi for Array { - ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. } +unsafe impl GodotFfi for Array { + ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; + fn from_sys; + fn sys; + fn from_sys_init; + // SAFETY: + // Nothing special needs to be done beyond a `std::mem::swap` when returning an Array. + fn move_return_ptr; + } + + // SAFETY: + // Arrays are properly initialized through a `from_sys` call, but the ref-count should be + // incremented as that is the callee's responsibility. + // + // Using `std::mem::forget(array.share())` increments the ref count. + unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::CallType) -> Self { + let array = Self::from_sys(ptr); + std::mem::forget(array.share()); + array + } unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { let mut result = Self::default(); diff --git a/godot-core/src/builtin/basis.rs b/godot-core/src/builtin/basis.rs index bf54e5736..bcf204c9a 100644 --- a/godot-core/src/builtin/basis.rs +++ b/godot-core/src/builtin/basis.rs @@ -570,7 +570,9 @@ impl Mul for Basis { } } -impl GodotFfi for Basis { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Basis { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/color.rs b/godot-core/src/builtin/color.rs index 788a40168..5714378df 100644 --- a/godot-core/src/builtin/color.rs +++ b/godot-core/src/builtin/color.rs @@ -311,7 +311,9 @@ impl Color { } } -impl GodotFfi for Color { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Color { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/dictionary.rs b/godot-core/src/builtin/dictionary.rs index 4c4d451d6..bf1845cee 100644 --- a/godot-core/src/builtin/dictionary.rs +++ b/godot-core/src/builtin/dictionary.rs @@ -239,8 +239,26 @@ impl Dictionary { // ---------------------------------------------------------------------------------------------------------------------------------------------- // Traits -impl GodotFfi for Dictionary { - ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. } +unsafe impl GodotFfi for Dictionary { + ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; + fn from_sys; + fn from_sys_init; + fn sys; + // SAFETY: + // Nothing special needs to be done beyond a `std::mem::swap` when returning a dictionary. + fn move_return_ptr; + } + + // SAFETY: + // Dictionaries are properly initialized through a `from_sys` call, but the ref-count should be + // incremented as that is the callee's responsibility. + // + // Using `std::mem::forget(dictionary.share())` increments the ref count. + unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::CallType) -> Self { + let dictionary = Self::from_sys(ptr); + std::mem::forget(dictionary.share()); + dictionary + } unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { let mut result = Self::default(); diff --git a/godot-core/src/builtin/macros.rs b/godot-core/src/builtin/macros.rs index 6173f1e23..5c1ccb65f 100644 --- a/godot-core/src/builtin/macros.rs +++ b/godot-core/src/builtin/macros.rs @@ -157,7 +157,10 @@ macro_rules! impl_builtin_stub { } } - impl GodotFfi for $Class { + // SAFETY: + // This is simply a wrapper around an `Opaque` value representing a value of the type. + // So this is safe. + unsafe impl GodotFfi for $Class { ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. } } }; diff --git a/godot-core/src/builtin/meta/signature.rs b/godot-core/src/builtin/meta/signature.rs index cce236dfe..f7c25d7a6 100644 --- a/godot-core/src/builtin/meta/signature.rs +++ b/godot-core/src/builtin/meta/signature.rs @@ -34,6 +34,7 @@ pub trait SignatureTuple { ret: sys::GDExtensionTypePtr, func: fn(&mut C, Self::Params) -> Self::Ret, method_name: &str, + call_type: sys::CallType, ); } @@ -143,6 +144,7 @@ macro_rules! impl_signature_for_tuple { ret: sys::GDExtensionTypePtr, func: fn(&mut C, Self::Params) -> Self::Ret, method_name: &str, + call_type: sys::CallType, ) { $crate::out!("ptrcall: {}", method_name); @@ -151,20 +153,20 @@ macro_rules! impl_signature_for_tuple { let args = ( $( unsafe { - <$Pn as sys::GodotFuncMarshal>::try_from_sys( - sys::force_mut_ptr(*args_ptr.offset($n)) + <$Pn as sys::GodotFuncMarshal>::try_from_arg( + sys::force_mut_ptr(*args_ptr.offset($n)), + call_type ) } .unwrap_or_else(|e| param_error::<$Pn>(method_name, $n, &e)), )* ); let ret_val = func(&mut *instance, args); - unsafe { <$R as sys::GodotFuncMarshal>::try_write_sys(&ret_val, ret) } - .unwrap_or_else(|e| return_error::<$R>(method_name, &e)); - - //Note: we want to prevent the destructor from being run, since we pass ownership to the caller. - //https://github.com/godot-rust/gdext/pull/165 - std::mem::forget(ret_val); + // SAFETY: + // `ret` is always a pointer to an initialized value of type $R + // TODO: double-check the above + <$R as sys::GodotFuncMarshal>::try_return(ret_val, ret, call_type) + .unwrap_or_else(|ret_val| return_error::<$R>(method_name, &ret_val)); } } }; diff --git a/godot-core/src/builtin/node_path.rs b/godot-core/src/builtin/node_path.rs index 942f33fe4..c6c032094 100644 --- a/godot-core/src/builtin/node_path.rs +++ b/godot-core/src/builtin/node_path.rs @@ -22,8 +22,26 @@ impl NodePath { } } -impl GodotFfi for NodePath { - ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. } +unsafe impl GodotFfi for NodePath { + ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; + fn from_sys; + fn sys; + fn from_sys_init; + // SAFETY: + // Nothing special needs to be done beyond a `std::mem::swap` when returning a NodePath. + fn move_return_ptr; + } + + // SAFETY: + // NodePaths are properly initialized through a `from_sys` call, but the ref-count should be + // incremented as that is the callee's responsibility. + // + // Using `std::mem::forget(node_path.share())` increments the ref count. + unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::CallType) -> Self { + let node_path = Self::from_sys(ptr); + std::mem::forget(node_path.clone()); + node_path + } unsafe fn from_sys_init_default(init_fn: impl FnOnce(GDExtensionTypePtr)) -> Self { let mut result = Self::default(); diff --git a/godot-core/src/builtin/packed_array.rs b/godot-core/src/builtin/packed_array.rs index 6a9e3c26e..54eb4850b 100644 --- a/godot-core/src/builtin/packed_array.rs +++ b/godot-core/src/builtin/packed_array.rs @@ -390,8 +390,26 @@ macro_rules! impl_packed_array { } } - impl GodotFfi for $PackedArray { - ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. } + unsafe impl GodotFfi for $PackedArray { + ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; + fn from_sys; + fn sys; + fn from_sys_init; + // SAFETY: + // Nothing special needs to be done beyond a `std::mem::swap` when returning a packed array. + fn move_return_ptr; + } + + // SAFETY: + // Packed arrays are properly initialized through a `from_sys` call, but the ref-count should be + // incremented as that is the callee's responsibility. + // + // Using `std::mem::forget(array.clone())` increments the ref count. + unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::CallType) -> Self { + let array = Self::from_sys(ptr); + std::mem::forget(array.clone()); + array + } unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { let mut result = Self::default(); diff --git a/godot-core/src/builtin/plane.rs b/godot-core/src/builtin/plane.rs index 259d537cc..c54e16b35 100644 --- a/godot-core/src/builtin/plane.rs +++ b/godot-core/src/builtin/plane.rs @@ -134,7 +134,9 @@ impl Neg for Plane { } } -impl GodotFfi for Plane { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Plane { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/projection.rs b/godot-core/src/builtin/projection.rs index 679c82adc..81260f779 100644 --- a/godot-core/src/builtin/projection.rs +++ b/godot-core/src/builtin/projection.rs @@ -486,7 +486,9 @@ impl GlamConv for Projection { type Glam = RMat4; } -impl GodotFfi for Projection { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Projection { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/quaternion.rs b/godot-core/src/builtin/quaternion.rs index 0de222d0c..98b76b548 100644 --- a/godot-core/src/builtin/quaternion.rs +++ b/godot-core/src/builtin/quaternion.rs @@ -254,7 +254,9 @@ impl Mul for Quaternion { } } -impl GodotFfi for Quaternion { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Quaternion { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/rect2.rs b/godot-core/src/builtin/rect2.rs index 73a109958..82828c613 100644 --- a/godot-core/src/builtin/rect2.rs +++ b/godot-core/src/builtin/rect2.rs @@ -104,6 +104,8 @@ impl Rect2 { */ } -impl GodotFfi for Rect2 { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Rect2 { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/rect2i.rs b/godot-core/src/builtin/rect2i.rs index 4fc611a5a..4945c49a8 100644 --- a/godot-core/src/builtin/rect2i.rs +++ b/godot-core/src/builtin/rect2i.rs @@ -93,6 +93,8 @@ impl Rect2i { */ } -impl GodotFfi for Rect2i { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Rect2i { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/rid.rs b/godot-core/src/builtin/rid.rs index 49723a8cb..7dbd99fee 100644 --- a/godot-core/src/builtin/rid.rs +++ b/godot-core/src/builtin/rid.rs @@ -81,6 +81,8 @@ impl Rid { } } -impl GodotFfi for Rid { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Rid { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/string.rs b/godot-core/src/builtin/string.rs index e97083f1e..498f48f71 100644 --- a/godot-core/src/builtin/string.rs +++ b/godot-core/src/builtin/string.rs @@ -35,7 +35,14 @@ impl GodotString { fn from_string_sys = from_sys; fn from_string_sys_init = from_sys_init; fn string_sys = sys; - fn write_string_sys = write_sys; + } + + /// Move `self` into a system pointer. + /// + /// # Safety + /// `dst` must be a pointer to a `GodotString` which is suitable for ffi with Godot. + pub unsafe fn move_string_ptr(self, dst: sys::GDExtensionStringPtr) { + self.move_return_ptr(dst as *mut _, sys::CallType::Standard); } /// Gets the internal chars slice from a [`GodotString`]. @@ -68,8 +75,26 @@ impl GodotString { } } -impl GodotFfi for GodotString { - ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. } +unsafe impl GodotFfi for GodotString { + ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; + fn from_sys; + fn sys; + fn from_sys_init; + // SAFETY: + // Nothing special needs to be done beyond a `std::mem::swap` when returning a GodotString. + fn move_return_ptr; + } + + // SAFETY: + // GodotStrings are properly initialized through a `from_sys` call, but the ref-count should be + // incremented as that is the callee's responsibility. + // + // Using `std::mem::forget(string.share())` increments the ref count. + unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::CallType) -> Self { + let string = Self::from_sys(ptr); + std::mem::forget(string.clone()); + string + } unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { let mut result = Self::default(); diff --git a/godot-core/src/builtin/string_name.rs b/godot-core/src/builtin/string_name.rs index 8d128b281..43b75fc3f 100644 --- a/godot-core/src/builtin/string_name.rs +++ b/godot-core/src/builtin/string_name.rs @@ -28,12 +28,29 @@ impl StringName { fn from_string_sys = from_sys; fn from_string_sys_init = from_sys_init; fn string_sys = sys; - fn write_string_sys = write_sys; } } -impl GodotFfi for StringName { - ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. } +unsafe impl GodotFfi for StringName { + ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; + fn from_sys; + fn sys; + fn from_sys_init; + // SAFETY: + // Nothing special needs to be done beyond a `std::mem::swap` when returning a StringName. + fn move_return_ptr; + } + + // SAFETY: + // StringNames are properly initialized through a `from_sys` call, but the ref-count should be + // incremented as that is the callee's responsibility. + // + // Using `std::mem::forget(string_name.share())` increments the ref count. + unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::CallType) -> Self { + let string_name = Self::from_sys(ptr); + std::mem::forget(string_name.clone()); + string_name + } unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { let mut result = Self::default(); diff --git a/godot-core/src/builtin/transform2d.rs b/godot-core/src/builtin/transform2d.rs index 7d6c94e4b..1d7ffe36d 100644 --- a/godot-core/src/builtin/transform2d.rs +++ b/godot-core/src/builtin/transform2d.rs @@ -358,7 +358,9 @@ impl GlamConv for Transform2D { type Glam = RAffine2; } -impl GodotFfi for Transform2D { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Transform2D { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/transform3d.rs b/godot-core/src/builtin/transform3d.rs index 5b4e4b284..f2459e4b6 100644 --- a/godot-core/src/builtin/transform3d.rs +++ b/godot-core/src/builtin/transform3d.rs @@ -352,7 +352,9 @@ impl GlamConv for Transform3D { type Glam = RAffine3; } -impl GodotFfi for Transform3D { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Transform3D { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs index 9957959d8..5f75b9327 100644 --- a/godot-core/src/builtin/variant/mod.rs +++ b/godot-core/src/builtin/variant/mod.rs @@ -193,7 +193,6 @@ impl Variant { fn from_var_sys = from_sys; fn from_var_sys_init = from_sys_init; fn var_sys = sys; - fn write_var_sys = write_sys; } #[doc(hidden)] @@ -215,7 +214,10 @@ impl Variant { } } -impl GodotFfi for Variant { +// SAFETY: +// `from_opaque` properly initializes a dereferenced pointer to an `OpaqueVariant`. +// `std::mem::swap` is sufficient for returning a value. +unsafe impl GodotFfi for Variant { ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. } unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { diff --git a/godot-core/src/builtin/vector2.rs b/godot-core/src/builtin/vector2.rs index 17af59e37..310f3d15b 100644 --- a/godot-core/src/builtin/vector2.rs +++ b/godot-core/src/builtin/vector2.rs @@ -310,7 +310,9 @@ impl_float_vector_fns!(Vector2, real); impl_vector_operators!(Vector2, real, (x, y)); impl_vector_index!(Vector2, real, (x, y), Vector2Axis, (X, Y)); -impl GodotFfi for Vector2 { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Vector2 { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } @@ -324,7 +326,9 @@ pub enum Vector2Axis { Y, } -impl GodotFfi for Vector2Axis { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Vector2Axis { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/vector2i.rs b/godot-core/src/builtin/vector2i.rs index b20a2dd87..b1109e7c5 100644 --- a/godot-core/src/builtin/vector2i.rs +++ b/godot-core/src/builtin/vector2i.rs @@ -88,7 +88,9 @@ impl_common_vector_fns!(Vector2i, i32); impl_vector_operators!(Vector2i, i32, (x, y)); impl_vector_index!(Vector2i, i32, (x, y), Vector2iAxis, (X, Y)); -impl GodotFfi for Vector2i { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Vector2i { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } @@ -102,6 +104,8 @@ pub enum Vector2iAxis { Y, } -impl GodotFfi for Vector2iAxis { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Vector2iAxis { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/vector3.rs b/godot-core/src/builtin/vector3.rs index 33d1f8d6d..cf83337c5 100644 --- a/godot-core/src/builtin/vector3.rs +++ b/godot-core/src/builtin/vector3.rs @@ -329,7 +329,9 @@ impl_float_vector_fns!(Vector3, real); impl_vector_operators!(Vector3, real, (x, y, z)); impl_vector_index!(Vector3, real, (x, y, z), Vector3Axis, (X, Y, Z)); -impl GodotFfi for Vector3 { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Vector3 { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } @@ -346,7 +348,9 @@ pub enum Vector3Axis { Z, } -impl GodotFfi for Vector3Axis { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Vector3Axis { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/vector3i.rs b/godot-core/src/builtin/vector3i.rs index dd7eaa69b..707f20bb4 100644 --- a/godot-core/src/builtin/vector3i.rs +++ b/godot-core/src/builtin/vector3i.rs @@ -97,7 +97,9 @@ impl_common_vector_fns!(Vector3i, i32); impl_vector_operators!(Vector3i, i32, (x, y, z)); impl_vector_index!(Vector3i, i32, (x, y, z), Vector3iAxis, (X, Y, Z)); -impl GodotFfi for Vector3i { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Vector3i { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } @@ -113,6 +115,8 @@ pub enum Vector3iAxis { Z, } -impl GodotFfi for Vector3iAxis { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Vector3iAxis { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/vector4.rs b/godot-core/src/builtin/vector4.rs index f57392099..de9f195a7 100644 --- a/godot-core/src/builtin/vector4.rs +++ b/godot-core/src/builtin/vector4.rs @@ -89,7 +89,9 @@ impl fmt::Display for Vector4 { } } -impl GodotFfi for Vector4 { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Vector4 { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } @@ -107,7 +109,9 @@ pub enum Vector4Axis { W, } -impl GodotFfi for Vector4Axis { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Vector4Axis { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/vector4i.rs b/godot-core/src/builtin/vector4i.rs index d8c341736..7e3bfb5b5 100644 --- a/godot-core/src/builtin/vector4i.rs +++ b/godot-core/src/builtin/vector4i.rs @@ -82,7 +82,9 @@ impl fmt::Display for Vector4i { } } -impl GodotFfi for Vector4i { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Vector4i { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } @@ -100,6 +102,8 @@ pub enum Vector4iAxis { W, } -impl GodotFfi for Vector4iAxis { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for Vector4iAxis { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/macros.rs b/godot-core/src/macros.rs index 26f76f969..1c7464f0a 100644 --- a/godot-core/src/macros.rs +++ b/godot-core/src/macros.rs @@ -112,6 +112,7 @@ macro_rules! gdext_register_method_inner { inst.$method_name( $( $param, )* ) }, stringify!($method_name), + sys::CallType::Standard ); } ); @@ -384,37 +385,23 @@ macro_rules! gdext_virtual_method_callback { }; } -#[doc(hidden)] #[macro_export] macro_rules! gdext_ptrcall { ( - $instance_ptr:ident, $args:ident, $ret:ident; + $instance_ptr:ident, $args_ptr:ident, $ret_ptr:ident; $Class:ty; fn $method_name:ident( $( $arg:ident : $ParamTy:ty, )* ) -> $( $RetTy:tt )+ ) => { - use $crate::sys; - - $crate::out!("ptrcall: {}", stringify!($method_name)); - let storage = $crate::private::as_storage::<$Class>($instance_ptr); - let mut instance = storage.get_mut(); - - let mut idx = 0; - $( - let $arg = <$ParamTy as sys::GodotFfi>::from_sys(sys::force_mut_ptr(*$args.offset(idx))); - // FIXME update refcount, e.g. Gd::ready() or T::Mem::maybe_inc_ref(&result); - // possibly in from_sys() directly; what about from_sys_init() and from_{obj|str}_sys()? - idx += 1; - )* - - let ret_val = instance.$method_name($( - $arg, - )*); - - <$($RetTy)+ as sys::GodotFfi>::write_sys(&ret_val, $ret); - // FIXME is inc_ref needed here? - // #[allow(clippy::forget_copy)] - // std::mem::forget(ret_val); + use $crate::builtin::meta::SignatureTuple; + <($($RetTy)+, $($ParamTy,)*) as SignatureTuple>::ptrcall::<$Class>( + $instance_ptr, + $args_ptr, + $ret_ptr, + |__instance, ($($arg,)*)| __instance.$method_name($($arg,)*), + stringify!($method_name), + sys::CallType::Virtual, + ) }; } diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 4544e183d..3b7ec0882 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -12,7 +12,7 @@ use std::ptr; use godot_ffi as sys; use godot_ffi::VariantType; use sys::types::OpaqueObject; -use sys::{ffi_methods, interface_fn, static_assert_eq_size, GodotFfi}; +use sys::{ffi_methods, interface_fn, static_assert_eq_size, CallType, GodotFfi}; use crate::builtin::meta::{ClassName, VariantMetadata}; use crate::builtin::{FromVariant, ToVariant, Variant, VariantConversionError}; @@ -395,7 +395,6 @@ impl Gd { fn from_obj_sys_weak = from_sys; fn obj_sys = sys; - fn write_obj_sys = write_sys; } /// Initializes this `Gd` from the object pointer as a **strong ref**, meaning @@ -501,8 +500,48 @@ where } } -impl GodotFfi for Gd { - ffi_methods! { type sys::GDExtensionTypePtr = Opaque; .. } +unsafe impl GodotFfi for Gd +where + T: GodotClass, +{ + ffi_methods! { type sys::GDExtensionTypePtr = Opaque; + fn from_sys; + fn from_sys_init; + fn sys; + } + + // For more context around `ref_get_object` and `ref_set_object`, see: + // https://github.com/godotengine/godot-cpp/issues/954 + + // SAFETY: + // When the `call_type` is `CallType::Virtual`, and the current type is known to inherit from `RefCounted` + // then we use `ref_get_object`. Otherwise we use `Gd::from_obj_sys`. + unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, call_type: CallType) -> Self { + match (T::Mem::is_static_ref_counted(), call_type) { + (true, CallType::Virtual) => { + let obj_ptr = interface_fn!(ref_get_object)(ptr as sys::GDExtensionRefPtr); + // ref_get_object increments the ref_count for us + Self::from_obj_sys_weak(obj_ptr) + } + _ => Self::from_obj_sys(ptr as sys::GDExtensionObjectPtr), + } + } + + // SAFETY: + // When the `call_type` is `CallType::Virtual`, and the current type is known to inherit from `RefCounted` + // then we use `ref_set_object`. Otherwise we use `std::ptr::write`. Finally we forget `self` as we pass + // ownership to the caller. + unsafe fn move_return_ptr(self, ptr: sys::GDExtensionTypePtr, call_type: CallType) { + match (T::Mem::is_static_ref_counted(), call_type) { + (true, CallType::Virtual) => { + interface_fn!(ref_set_object)(ptr as sys::GDExtensionRefPtr, self.obj_sys()) + } + + _ => std::ptr::write(ptr as *mut _, self.opaque), + } + // We've passed ownership to caller. + std::mem::forget(self); + } } impl Gd { diff --git a/godot-core/src/obj/instance_id.rs b/godot-core/src/obj/instance_id.rs index 5d94acb86..4144bdbb8 100644 --- a/godot-core/src/obj/instance_id.rs +++ b/godot-core/src/obj/instance_id.rs @@ -77,7 +77,9 @@ impl Debug for InstanceId { } } -impl GodotFfi for InstanceId { +// SAFETY: +// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +unsafe impl GodotFfi for InstanceId { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/obj/traits.rs b/godot-core/src/obj/traits.rs index 4c2579508..abb1c0cba 100644 --- a/godot-core/src/obj/traits.rs +++ b/godot-core/src/obj/traits.rs @@ -245,6 +245,10 @@ pub mod mem { /// Check if ref-counted, return `None` if information is not available (dynamic and obj dead) fn is_ref_counted(obj: &Gd) -> Option; + + fn is_static_ref_counted() -> bool { + false + } } pub trait PossiblyManual {} @@ -281,6 +285,10 @@ pub mod mem { fn is_ref_counted(_obj: &Gd) -> Option { Some(true) } + + fn is_static_ref_counted() -> bool { + true + } } /// Memory managed through Godot reference counter, if present; otherwise manual. diff --git a/godot-core/src/registry.rs b/godot-core/src/registry.rs index 73cf88254..27bb31eed 100644 --- a/godot-core/src/registry.rs +++ b/godot-core/src/registry.rs @@ -350,9 +350,8 @@ pub mod callbacks { let instance = storage.get(); let string = ::__godot_to_string(&*instance); - // Transfer ownership to Godot, disable destructor - string.write_string_sys(out_string); - std::mem::forget(string); + // Transfer ownership to Godot + string.move_string_ptr(out_string); } pub unsafe extern "C" fn reference(instance: sys::GDExtensionClassInstancePtr) { diff --git a/godot-ffi/src/godot_ffi.rs b/godot-ffi/src/godot_ffi.rs index 9187fc295..dbb8778d6 100644 --- a/godot-ffi/src/godot_ffi.rs +++ b/godot-ffi/src/godot_ffi.rs @@ -9,12 +9,19 @@ use std::fmt::Debug; /// Adds methods to convert from and to Godot FFI pointers. /// See [crate::ffi_methods] for ergonomic implementation. -pub trait GodotFfi { +/// +/// # Safety +/// +/// [`from_arg_ptr`](GodotFfi::from_arg_ptr) and [`move_return_ptr`](GodotFfi::move_return_ptr) +/// must properly initialize and clean up values given the [`CallType`] provided by the caller. +pub unsafe trait GodotFfi { /// Construct from Godot opaque pointer. /// /// # Safety /// `ptr` must be a valid _type ptr_: it must follow Godot's convention to encode `Self`, /// which is different depending on the type. + /// The type in `ptr` must not require any special consideration upon referencing. Such as + /// incrementing a refcount. unsafe fn from_sys(ptr: sys::GDExtensionTypePtr) -> Self; /// Construct uninitialized opaque data, then initialize it with `init_fn` function. @@ -67,12 +74,43 @@ pub trait GodotFfi { self.sys() } - /// Write the contents of `self` into the pointer `dst`. + /// Construct from a pointer to an argument in a call. /// /// # Safety - /// `dst` must be a valid _type ptr_: it must follow Godot's convention to encode `Self`, + /// `ptr` must be a valid _type ptr_: it must follow Godot's convention to encode `Self`, /// which is different depending on the type. - unsafe fn write_sys(&self, dst: sys::GDExtensionTypePtr); + /// `ptr` must encode `Self` according to the given `call_type`'s encoding of argument values. + unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, call_type: CallType) -> Self; + + /// Move self into the pointer in pointer `dst`, dropping what is already in `dst. + /// + /// # Safety + /// `dst` must be a valid _type ptr_: it must follow Godot's convention to encode `Self`, + /// hich is different depending on the type. + /// `dst` must be able to accept a value of type `Self` encoded according to the given + /// `call_type`'s encoding of return values. + unsafe fn move_return_ptr(self, dst: sys::GDExtensionTypePtr, call_type: CallType); +} + +/// An indication of what type of pointer call is being made. +#[derive(Default, Copy, Clone, Eq, PartialEq, Debug)] +pub enum CallType { + /// Standard pointer call. + /// + /// In a standard ptrcall, every argument is passed in as a pointer to a value of that type, and the + /// return value must be moved into the return pointer. + #[default] + Standard, + /// Virtual pointer call. + /// + /// A virtual call behaves like [`CallType::Standard`], except for `RefCounted` objects. + /// `RefCounted` objects are instead passed in and returned as `Ref` objects. + /// + /// To properly get a value from an argument in a pointer call, you must use `ref_get_object`. And to + /// return a value you must use `ref_set_object`. + /// + /// See also https://github.com/godotengine/godot-cpp/issues/954. + Virtual, } /// Trait implemented for all types that can be passed to and from user-defined `#[func]` methods @@ -85,13 +123,23 @@ pub trait GodotFuncMarshal: Sized { /// /// # Safety /// The value behind `ptr` must be the C FFI type that corresponds to `Self`. - unsafe fn try_from_sys(ptr: sys::GDExtensionTypePtr) -> Result; + /// See also [`GodotFfi::from_arg_ptr`]. + unsafe fn try_from_arg( + ptr: sys::GDExtensionTypePtr, + call_type: CallType, + ) -> Result; /// Used for function return values. On failure, `self` which can't be converted to Via is returned. /// /// # Safety /// The value behind `ptr` must be the C FFI type that corresponds to `Self`. - unsafe fn try_write_sys(&self, dst: sys::GDExtensionTypePtr) -> Result<(), Self>; + /// `dst` must point to an initialized value of type `Via`. + /// See also [`GodotFfi::move_return_ptr`]. + unsafe fn try_return( + self, + dst: sys::GDExtensionTypePtr, + call_type: CallType, + ) -> Result<(), Self>; } // ---------------------------------------------------------------------------------------------------------------------------------------------- @@ -125,11 +173,16 @@ macro_rules! ffi_methods_one { &self.opaque as *const _ as $Ptr } }; - (OpaquePtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $write_sys:ident = write_sys) => { + (OpaquePtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $from_arg_ptr:ident = from_arg_ptr) => { + $( #[$attr] )? $vis + unsafe fn $from_arg_ptr(ptr: $Ptr, _call_type: $crate::CallType) -> Self { + Self::from_sys(ptr as *mut _) + } + }; + (OpaquePtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $move_return_ptr:ident = move_return_ptr) => { $( #[$attr] )? $vis - unsafe fn $write_sys(&self, dst: $Ptr) { - // Note: this is the same impl as for impl_ffi_as_opaque_value, which is... interesting - std::ptr::write(dst as *mut _, self.opaque) + unsafe fn $move_return_ptr(mut self, dst: $Ptr, _call_type: $crate::CallType) { + std::ptr::swap(dst as *mut _, std::ptr::addr_of_mut!(self.opaque)) } }; @@ -155,11 +208,16 @@ macro_rules! ffi_methods_one { unsafe { std::mem::transmute(self.opaque) } } }; - (OpaqueValue $Ptr:ty; $( #[$attr:meta] )? $vis:vis $write_sys:ident = write_sys) => { + (OpaqueValue $Ptr:ty; $( #[$attr:meta] )? $vis:vis $from_arg_ptr:ident = from_arg_ptr) => { + $( #[$attr] )? $vis + unsafe fn $from_arg_ptr(ptr: $Ptr, _call_type: $crate::CallType) -> Self { + Self::from_sys(ptr as *mut _) + } + }; + (OpaqueValue $Ptr:ty; $( #[$attr:meta] )? $vis:vis $move_return_ptr:ident = move_return_ptr) => { $( #[$attr] )? $vis - unsafe fn $write_sys(&self, dst: $Ptr) { - // Note: this is the same impl as for impl_ffi_as_opaque_value, which is... interesting - std::ptr::write(dst as *mut _, self.opaque); + unsafe fn $move_return_ptr(mut self, dst: $Ptr, _call_type: $crate::CallType) { + std::ptr::swap(dst, std::mem::transmute::<_, $Ptr>(self.opaque)) } }; @@ -185,10 +243,16 @@ macro_rules! ffi_methods_one { self as *const Self as $Ptr } }; - (SelfPtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $write_sys:ident = write_sys) => { + (SelfPtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $from_arg_ptr:ident = from_arg_ptr) => { + $( #[$attr] )? $vis + unsafe fn $from_arg_ptr(ptr: $Ptr, _call_type: $crate::CallType) -> Self { + *(ptr as *mut Self) + } + }; + (SelfPtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $move_return_ptr:ident = move_return_ptr) => { $( #[$attr] )? $vis - unsafe fn $write_sys(&self, dst: $Ptr) { - *(dst as *mut Self) = *self; + unsafe fn $move_return_ptr(self, dst: $Ptr, _call_type: $crate::CallType) { + *(dst as *mut Self) = self } }; } @@ -208,13 +272,14 @@ macro_rules! ffi_methods_rest { $( $crate::ffi_methods_one!($Impl $Ptr; $sys_fn = $sys_fn); )* }; - ( // impl GodotFfi for T (default all 4) + ( // impl GodotFfi for T (default all 5) $Impl:ident $Ptr:ty; .. ) => { $crate::ffi_methods_one!($Impl $Ptr; from_sys = from_sys); $crate::ffi_methods_one!($Impl $Ptr; from_sys_init = from_sys_init); $crate::ffi_methods_one!($Impl $Ptr; sys = sys); - $crate::ffi_methods_one!($Impl $Ptr; write_sys = write_sys); + $crate::ffi_methods_one!($Impl $Ptr; from_arg_ptr = from_arg_ptr); + $crate::ffi_methods_one!($Impl $Ptr; move_return_ptr = move_return_ptr); }; } @@ -236,6 +301,26 @@ macro_rules! ffi_methods_rest { /// The address of `Self` is directly reinterpreted as the sys pointer. /// The size of the corresponding sys type (the `N` in `Opaque*`) must not be bigger than `size_of::()`. /// This cannot be checked easily, because Self cannot be used in size_of(). There would of course be workarounds. +/// +/// Using this macro as a complete implementation for [`GodotFfi`] is sound only when: +/// +/// ## Using `*mut Opaque` +/// +/// Turning pointer call arguments into a value is simply calling `from_opaque` on the +/// dereferenced argument pointer. +/// Returning a value from a pointer call is simply calling [`std::ptr::swap`] on the return pointer +/// and the address to the `opaque` field. +/// +/// ## Using `Opaque` +/// +/// Turning pointer call arguments into a value is simply calling `from_opaue` on the argument pointer. +/// Returning a value from a pointer call is simply calling [`std::ptr::swap`] on the return pointer +/// and the `opaque` field transmuted into a pointer. +/// +/// ## Using `*mut Self` +/// +/// Turning pointer call arguments into a value is a dereference. +/// Returning a value from a pointer call is `*ret_ptr = value`. #[macro_export] macro_rules! ffi_methods { ( // Sys pointer = address of opaque @@ -263,13 +348,15 @@ macro_rules! ffi_methods { // ---------------------------------------------------------------------------------------------------------------------------------------------- // Implementation for common types (needs to be this crate due to orphan rule) mod scalars { - use super::{GodotFfi, GodotFuncMarshal}; + use super::{CallType, GodotFfi, GodotFuncMarshal}; use crate as sys; use std::convert::Infallible; macro_rules! impl_godot_marshalling { ($T:ty) => { - impl GodotFfi for $T { + // SAFETY: + // This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. + unsafe impl GodotFfi for $T { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } }; @@ -281,37 +368,23 @@ mod scalars { impl GodotFuncMarshal for $T { type Via = $Via; - unsafe fn try_from_sys(ptr: sys::GDExtensionTypePtr) -> Result { - let via = <$Via as GodotFfi>::from_sys(ptr); - Self::try_from(via).map_err(|_| via) - } - - unsafe fn try_write_sys(&self, dst: sys::GDExtensionTypePtr) -> Result<(), Self> { - <$Via>::try_from(*self) - .and_then(|via| { - <$Via as GodotFfi>::write_sys(&via, dst); - Ok(()) - }) - .map_err(|_| *self) - } - } - }; - - ($T:ty as $Via:ty; lossy) => { - // implicit bounds: - // T: TryFrom, Copy - // Via: TryFrom, GodotFfi - impl GodotFuncMarshal for $T { - type Via = $Via; - - unsafe fn try_from_sys(ptr: sys::GDExtensionTypePtr) -> Result { - let via = <$Via as GodotFfi>::from_sys(ptr); + unsafe fn try_from_arg( + ptr: sys::GDExtensionTypePtr, + call_type: CallType, + ) -> Result { + let via = <$Via as GodotFfi>::from_arg_ptr(ptr, call_type); + // Godot does not always give us a valid `i64`/`f64`, nor `Self` when it does a pointer + // call, so we cannot use `try_from` here. Ok(via as Self) } - unsafe fn try_write_sys(&self, dst: sys::GDExtensionTypePtr) -> Result<(), Self> { - let via = *self as $Via; - <$Via as GodotFfi>::write_sys(&via, dst); + unsafe fn try_return( + self, + dst: sys::GDExtensionTypePtr, + call_type: CallType, + ) -> Result<(), Self> { + let via = <$Via>::try_from(self).map_err(|_| self)?; + via.move_return_ptr(dst, call_type); Ok(()) } } @@ -325,15 +398,14 @@ mod scalars { // Only implements GodotFuncMarshal impl_godot_marshalling!(i32 as i64); - impl_godot_marshalling!(u32 as i64); impl_godot_marshalling!(i16 as i64); - impl_godot_marshalling!(u16 as i64); impl_godot_marshalling!(i8 as i64); + impl_godot_marshalling!(u32 as i64); + impl_godot_marshalling!(u16 as i64); impl_godot_marshalling!(u8 as i64); + impl_godot_marshalling!(f32 as f64); - impl_godot_marshalling!(f32 as f64; lossy); - - impl GodotFfi for () { + unsafe impl GodotFfi for () { unsafe fn from_sys(_ptr: sys::GDExtensionTypePtr) -> Self { // Do nothing } @@ -347,7 +419,18 @@ mod scalars { self as *const _ as sys::GDExtensionTypePtr } - unsafe fn write_sys(&self, _dst: sys::GDExtensionTypePtr) { + // SAFETY: + // We're not accessing the value in `_ptr`. + unsafe fn from_arg_ptr(_ptr: sys::GDExtensionTypePtr, _call_type: super::CallType) -> Self { + } + + // SAFETY: + // We're not doing anything with `_dst`. + unsafe fn move_return_ptr( + self, + _dst: sys::GDExtensionTypePtr, + _call_type: super::CallType, + ) { // Do nothing } } @@ -358,12 +441,19 @@ mod scalars { { type Via = Infallible; - unsafe fn try_from_sys(ptr: sys::GDExtensionTypePtr) -> Result { - Ok(Self::from_sys(ptr)) + unsafe fn try_from_arg( + ptr: sys::GDExtensionTypePtr, + call_type: CallType, + ) -> Result { + Ok(Self::from_arg_ptr(ptr, call_type)) } - unsafe fn try_write_sys(&self, dst: sys::GDExtensionTypePtr) -> Result<(), Self> { - self.write_sys(dst); + unsafe fn try_return( + self, + dst: sys::GDExtensionTypePtr, + call_type: CallType, + ) -> Result<(), Self> { + self.move_return_ptr(dst, call_type); Ok(()) } } diff --git a/godot-ffi/src/lib.rs b/godot-ffi/src/lib.rs index 7ba644997..a4a07478f 100644 --- a/godot-ffi/src/lib.rs +++ b/godot-ffi/src/lib.rs @@ -28,7 +28,7 @@ mod plugins; #[doc(hidden)] pub use paste; -pub use crate::godot_ffi::{GodotFfi, GodotFuncMarshal}; +pub use crate::godot_ffi::{CallType, GodotFfi, GodotFuncMarshal}; pub use gen::central::*; pub use gen::gdextension_interface::*; diff --git a/itest/godot/ManualFfiTests.gd b/itest/godot/ManualFfiTests.gd index ecb6ae843..4b184c72c 100644 --- a/itest/godot/ManualFfiTests.gd +++ b/itest/godot/ManualFfiTests.gd @@ -72,22 +72,116 @@ func test_export(): obj.free() node.free() -func test_untyped_array_pass_to_user_func(): +func test_untyped_array_pass_to_user_func_varcall(): var obj = ArrayTest.new() var array: Array = [42, "answer"] assert_eq(obj.pass_untyped_array(array), 2) -func test_untyped_array_return_from_user_func(): +func test_untyped_array_pass_to_user_func_ptrcall(): + var obj: ArrayTest = ArrayTest.new() + var array: Array = [42, "answer"] + assert_eq(obj.pass_untyped_array(array), 2) + +func test_untyped_array_return_from_user_func_varcall(): var obj = ArrayTest.new() var array: Array = obj.return_untyped_array() assert_eq(array, [42, "answer"]) -func test_typed_array_pass_to_user_func(): +func test_untyped_array_return_from_user_func_ptrcall(): + var obj: ArrayTest = ArrayTest.new() + var array: Array = obj.return_untyped_array() + assert_eq(array, [42, "answer"]) + +func test_typed_array_pass_to_user_func_varcall(): var obj = ArrayTest.new() var array: Array[int] = [1, 2, 3] assert_eq(obj.pass_typed_array(array), 6) -func test_typed_array_return_from_user_func(): +func test_typed_array_pass_to_user_func_ptrcall(): + var obj: ArrayTest = ArrayTest.new() + var array: Array[int] = [1, 2, 3] + assert_eq(obj.pass_typed_array(array), 6) + +func test_typed_array_return_from_user_func_varcall(): var obj = ArrayTest.new() var array: Array[int] = obj.return_typed_array(3) assert_eq(array, [1, 2, 3]) + +func test_typed_array_return_from_user_func_ptrcall(): + var obj: ArrayTest = ArrayTest.new() + var array: Array[int] = obj.return_typed_array(3) + assert_eq(array, [1, 2, 3]) + +class MockObjGd extends Object: + var i: int = 0 + + func _init(i: int): + self.i = i + +func test_object_pass_to_user_func_varcall(): + var obj_test = ObjectTest.new() + var obj: MockObjGd = MockObjGd.new(10) + assert_eq(obj_test.pass_object(obj), 10) + +func test_object_pass_to_user_func_ptrcall(): + var obj_test: ObjectTest = ObjectTest.new() + var obj: MockObjGd = MockObjGd.new(10) + assert_eq(obj_test.pass_object(obj), 10) + +func test_object_return_from_user_func_varcall(): + var obj_test = ObjectTest.new() + var obj: MockObjRust = obj_test.return_object() + assert_eq(obj.i, 42) + obj.free() + +func test_object_return_from_user_func_ptrcall(): + var obj_test: ObjectTest = ObjectTest.new() + var obj: MockObjRust = obj_test.return_object() + assert_eq(obj.i, 42) + obj.free() + +class MockRefCountedGd extends RefCounted: + var i: int = 0 + + func _init(i: int): + self.i = i + +func test_refcounted_pass_to_user_func_varcall(): + var obj_test = ObjectTest.new() + var obj: MockRefCountedGd = MockRefCountedGd.new(10) + assert_eq(obj_test.pass_refcounted(obj), 10) + +func test_refcounted_pass_to_user_func_ptrcall(): + var obj_test: ObjectTest = ObjectTest.new() + var obj: MockRefCountedGd = MockRefCountedGd.new(10) + assert_eq(obj_test.pass_refcounted(obj), 10) + +func test_refcounted_as_object_pass_to_user_func_varcall(): + var obj_test = ObjectTest.new() + var obj: MockRefCountedGd = MockRefCountedGd.new(10) + assert_eq(obj_test.pass_refcounted_as_object(obj), 10) + +func test_refcounted_as_object_pass_to_user_func_ptrcall(): + var obj_test: ObjectTest = ObjectTest.new() + var obj: MockRefCountedGd = MockRefCountedGd.new(10) + assert_eq(obj_test.pass_refcounted_as_object(obj), 10) + +func test_refcounted_return_from_user_func_varcall(): + var obj_test = ObjectTest.new() + var obj: MockRefCountedRust = obj_test.return_refcounted() + assert_eq(obj.i, 42) + +func test_refcounted_return_from_user_func_ptrcall(): + var obj_test: ObjectTest = ObjectTest.new() + var obj: MockRefCountedRust = obj_test.return_refcounted() + assert_eq(obj.i, 42) + +func test_refcounted_as_object_return_from_user_func_varcall(): + var obj_test = ObjectTest.new() + var obj: MockRefCountedRust = obj_test.return_refcounted_as_object() + assert_eq(obj.i, 42) + +func test_refcounted_as_object_return_from_user_func_ptrcall(): + var obj_test: ObjectTest = ObjectTest.new() + var obj: MockRefCountedRust = obj_test.return_refcounted_as_object() + assert_eq(obj.i, 42) \ No newline at end of file diff --git a/itest/godot/TestRunner.gd b/itest/godot/TestRunner.gd index 3e28abe2f..9466ecf99 100644 --- a/itest/godot/TestRunner.gd +++ b/itest/godot/TestRunner.gd @@ -38,7 +38,9 @@ func _ready(): gdscript_tests, gdscript_suites.size(), allow_focus, + get_tree().root, self, + get_tree() ) var exit_code: int = 0 if success else 1 diff --git a/itest/rust/build.rs b/itest/rust/build.rs index e6bfe1241..6a399a26d 100644 --- a/itest/rust/build.rs +++ b/itest/rust/build.rs @@ -45,8 +45,11 @@ fn collect_inputs() -> Vec { // Scalar push!(inputs; int, i64, -922337203685477580); push!(inputs; int, i32, -2147483648); + push!(inputs; int, u32, 4294967295); push!(inputs; int, i16, -32767); + push!(inputs; int, u16, 65535); push!(inputs; int, i8, -128); + push!(inputs; int, u8, 255); push!(inputs; float, f32, 12.5); push!(inputs; float, f64, 127.83156478); push!(inputs; bool, bool, true); diff --git a/itest/rust/src/lib.rs b/itest/rust/src/lib.rs index 9596878f6..17670ef25 100644 --- a/itest/rust/src/lib.rs +++ b/itest/rust/src/lib.rs @@ -4,9 +4,10 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use godot::engine::{Engine, Node}; +use godot::engine::{Engine, Node, Window}; use godot::init::{gdextension, ExtensionLibrary}; use godot::obj::Gd; +use godot::prelude::SceneTree; use godot::sys; mod array_test; @@ -104,7 +105,9 @@ fn collect_rust_tests() -> (Vec, usize, bool) { } pub struct TestContext { - scene_tree: Gd, + _scene_tree: Gd, + root_node: Gd, + test_node: Gd, } #[derive(Copy, Clone)] diff --git a/itest/rust/src/node_test.rs b/itest/rust/src/node_test.rs index 5099ed647..2daf9189d 100644 --- a/itest/rust/src/node_test.rs +++ b/itest/rust/src/node_test.rs @@ -58,7 +58,7 @@ fn node_get_node_fail() { #[itest] fn node_path_from_str(ctx: &TestContext) { - let child = ctx.scene_tree.share(); + let child = ctx.test_node.share(); assert_eq!( child.get_path().to_string(), NodePath::from_str("/root/TestRunner").unwrap().to_string() @@ -96,7 +96,7 @@ fn node_scene_tree() { // FIXME: call_group() crashes #[itest(skip)] fn node_call_group(ctx: &TestContext) { - let mut node = ctx.scene_tree.share(); + let mut node = ctx.test_node.share(); let mut tree = node.get_tree().unwrap(); node.add_to_group("group".into(), true); diff --git a/itest/rust/src/object_test.rs b/itest/rust/src/object_test.rs index b7c160ccc..b1dd8de2a 100644 --- a/itest/rust/src/object_test.rs +++ b/itest/rust/src/object_test.rs @@ -18,7 +18,7 @@ use godot::engine::{ }; use godot::obj::{Base, Gd, InstanceId}; use godot::obj::{Inherits, Share}; -use godot::sys::GodotFfi; +use godot::sys::{self, GodotFfi}; use crate::{expect_panic, itest, TestContext}; @@ -94,8 +94,9 @@ fn object_user_roundtrip_write() { let obj: Gd = Gd::new(user); assert_eq!(obj.bind().value, value); - let obj2 = unsafe { Gd::::from_sys_init(|ptr| obj.write_sys(ptr)) }; - std::mem::forget(obj); + let obj2 = unsafe { + Gd::::from_sys_init(|ptr| obj.move_return_ptr(ptr, sys::CallType::Standard)) + }; assert_eq!(obj2.bind().value, value); } // drop @@ -614,7 +615,7 @@ fn object_call_with_args() { fn object_get_scene_tree(ctx: &TestContext) { let node = Node3D::new_alloc(); - let mut tree = ctx.scene_tree.share(); + let mut tree = ctx.test_node.share(); tree.add_child(node.upcast(), false, InternalMode::INTERNAL_MODE_DISABLED); let count = tree.get_child_count(false); @@ -655,3 +656,66 @@ impl Drop for Tracker { *self.drop_count.borrow_mut() += 1; } } + +pub mod object_test_gd { + use godot::prelude::*; + + #[derive(GodotClass)] + #[class(init, base=Object)] + struct MockObjRust { + #[export] + i: i64, + } + + #[godot_api] + impl MockObjRust {} + + #[derive(GodotClass)] + #[class(init, base=RefCounted)] + struct MockRefCountedRust { + #[export] + i: i64, + } + + #[godot_api] + impl MockRefCountedRust {} + + #[derive(GodotClass, Debug)] + #[class(init, base=RefCounted)] + struct ObjectTest; + + #[godot_api] + impl ObjectTest { + #[func] + fn pass_object(&self, object: Gd) -> i64 { + let i = object.get("i".into()).to(); + object.free(); + i + } + + #[func] + fn return_object(&self) -> Gd { + Gd::new(MockObjRust { i: 42 }).upcast() + } + + #[func] + fn pass_refcounted(&self, object: Gd) -> i64 { + object.get("i".into()).to() + } + + #[func] + fn pass_refcounted_as_object(&self, object: Gd) -> i64 { + object.get("i".into()).to() + } + + #[func] + fn return_refcounted(&self) -> Gd { + Gd::new(MockRefCountedRust { i: 42 }).upcast() + } + + #[func] + fn return_refcounted_as_object(&self) -> Gd { + Gd::new(MockRefCountedRust { i: 42 }).upcast() + } + } +} diff --git a/itest/rust/src/runner.rs b/itest/rust/src/runner.rs index 70c452f73..30369b475 100644 --- a/itest/rust/src/runner.rs +++ b/itest/rust/src/runner.rs @@ -8,8 +8,9 @@ use std::time::{Duration, Instant}; use godot::bind::{godot_api, GodotClass}; use godot::builtin::{ToVariant, Variant, VariantArray}; -use godot::engine::Node; +use godot::engine::{Node, Window}; use godot::obj::Gd; +use godot::prelude::SceneTree; use crate::{RustTestCase, TestContext}; @@ -31,7 +32,9 @@ impl IntegrationTests { gdscript_tests: VariantArray, gdscript_file_count: i64, allow_focus: bool, - scene_tree: Gd, + root_node: Gd, + test_node: Gd, + scene_tree: Gd, ) -> bool { println!("{}Run{} Godot integration tests...", FMT_CYAN_BOLD, FMT_END); @@ -54,7 +57,7 @@ impl IntegrationTests { } let clock = Instant::now(); - self.run_rust_tests(rust_tests, scene_tree); + self.run_rust_tests(rust_tests, root_node, test_node, scene_tree); let rust_time = clock.elapsed(); let gdscript_time = if !focus_run { self.run_gdscript_tests(gdscript_tests); @@ -66,8 +69,18 @@ impl IntegrationTests { self.conclude(rust_time, gdscript_time, allow_focus) } - fn run_rust_tests(&mut self, tests: Vec, scene_tree: Gd) { - let ctx = TestContext { scene_tree }; + fn run_rust_tests( + &mut self, + tests: Vec, + root_node: Gd, + test_node: Gd, + scene_tree: Gd, + ) { + let ctx = TestContext { + _scene_tree: scene_tree, + root_node, + test_node, + }; let mut last_file = None; for test in tests { diff --git a/itest/rust/src/variant_test.rs b/itest/rust/src/variant_test.rs index d5b69ec1a..06d0e1197 100644 --- a/itest/rust/src/variant_test.rs +++ b/itest/rust/src/variant_test.rs @@ -254,7 +254,12 @@ fn variant_sys_conversion2() { let mut buffer = [0u8; 50]; let v = Variant::from(7); - unsafe { v.write_sys(buffer.as_mut_ptr() as sys::GDExtensionTypePtr) }; + unsafe { + v.clone().move_return_ptr( + buffer.as_mut_ptr() as sys::GDExtensionTypePtr, + sys::CallType::Standard, + ) + }; let v2 = unsafe { Variant::from_sys_init(|ptr| { diff --git a/itest/rust/src/virtual_methods_test.rs b/itest/rust/src/virtual_methods_test.rs index 14c19c247..e17c5be60 100644 --- a/itest/rust/src/virtual_methods_test.rs +++ b/itest/rust/src/virtual_methods_test.rs @@ -10,7 +10,10 @@ use crate::TestContext; use godot::bind::{godot_api, GodotClass}; use godot::builtin::GodotString; use godot::engine::node::InternalMode; -use godot::engine::{Node, Node2D, Node2DVirtual, NodeVirtual, RefCounted, RefCountedVirtual}; +use godot::engine::{ + InputEvent, InputEventAction, Node, Node2D, Node2DVirtual, NodeVirtual, RefCounted, + RefCountedVirtual, +}; use godot::obj::{Base, Gd, Share}; use godot::prelude::PackedStringArray; use godot::test::itest; @@ -112,6 +115,25 @@ impl NodeVirtual for ReturnVirtualTest { } } +#[class(base=Node2D)] +struct InputVirtualTest { + #[base] + base: Base, + event: Option>, +} + +#[godot_api] +impl Node2DVirtual for InputVirtualTest { + fn init(base: Base) -> Self { + InputVirtualTest { base, event: None } + } + + fn input(&mut self, event: Gd) { + println!("nya"); + self.event = Some(event); + } +} + // ---------------------------------------------------------------------------------------------------------------------------------------------- #[itest] @@ -125,7 +147,7 @@ fn test_ready(test_context: &TestContext) { assert_eq!(obj.bind().implementation_value, 0); // Add to scene tree - let mut test_node = test_context.scene_tree.share(); + let mut test_node = test_context.test_node.share(); test_node.add_child( obj.share().upcast(), false, @@ -141,7 +163,7 @@ fn test_ready_multiple_fires(test_context: &TestContext) { let obj = Gd::::new_default(); assert_eq!(obj.bind().implementation_value, 0); - let mut test_node = test_context.scene_tree.share(); + let mut test_node = test_context.test_node.share(); // Add to scene tree test_node.add_child( @@ -170,7 +192,7 @@ fn test_ready_request_ready(test_context: &TestContext) { let obj = Gd::::new_default(); assert_eq!(obj.bind().implementation_value, 0); - let mut test_node = test_context.scene_tree.share(); + let mut test_node = test_context.test_node.share(); // Add to scene tree test_node.add_child( @@ -213,7 +235,7 @@ fn test_tree_enters_exits(test_context: &TestContext) { let obj = Gd::::new_default(); assert_eq!(obj.bind().tree_enters, 0); assert_eq!(obj.bind().tree_exits, 0); - let mut test_node = test_context.scene_tree.share(); + let mut test_node = test_context.test_node.share(); // Add to scene tree test_node.add_child( @@ -245,3 +267,26 @@ fn test_virtual_method_with_return(_test_context: &TestContext) { assert_eq!(output.len(), 1); obj.free(); } + +fn test_input_event(test_context: &TestContext) { + let obj = Gd::::new_default(); + assert_eq!(obj.bind().event, None); + + test_context.test_node.share().add_child( + obj.share().upcast(), + false, + InternalMode::INTERNAL_MODE_DISABLED, + ); + + let mut event = InputEventAction::new(); + event.set_action("debug".into()); + event.set_pressed(true); + + // We're running in headless mode, so Input.parse_input_event does not work + test_context + .root_node + .share() + .push_input(event.share().upcast(), false); + + assert_eq!(obj.bind().event, Some(event.upcast())); +} From 820669791415d3fbd09e3a3a234c5785a073d6b2 Mon Sep 17 00:00:00 2001 From: Lili Zoey Date: Mon, 10 Apr 2023 22:28:22 +0200 Subject: [PATCH 6/6] Fix return virtual method type Add another virtual method return test Make test that causes memory leak use `#[itest(skip)]` Move the logic for determining whether to use `Ref` or not entirely into `Mem` Remove some unnecessary manual ffi tests Rename CallType --- godot-codegen/src/central_generator.rs | 4 +- godot-codegen/src/lib.rs | 4 + godot-codegen/src/util.rs | 2 + godot-core/src/builtin/aabb.rs | 2 +- godot-core/src/builtin/array.rs | 17 +-- godot-core/src/builtin/basis.rs | 2 +- godot-core/src/builtin/color.rs | 2 +- godot-core/src/builtin/dictionary.rs | 18 +-- godot-core/src/builtin/meta/signature.rs | 4 +- godot-core/src/builtin/node_path.rs | 18 +-- godot-core/src/builtin/packed_array.rs | 2 +- godot-core/src/builtin/plane.rs | 2 +- godot-core/src/builtin/projection.rs | 2 +- godot-core/src/builtin/quaternion.rs | 2 +- godot-core/src/builtin/rect2.rs | 2 +- godot-core/src/builtin/rect2i.rs | 2 +- godot-core/src/builtin/rid.rs | 2 +- godot-core/src/builtin/string.rs | 22 +-- godot-core/src/builtin/string_name.rs | 18 +-- godot-core/src/builtin/transform2d.rs | 2 +- godot-core/src/builtin/transform3d.rs | 2 +- godot-core/src/builtin/vector2.rs | 4 +- godot-core/src/builtin/vector2i.rs | 4 +- godot-core/src/builtin/vector3.rs | 4 +- godot-core/src/builtin/vector3i.rs | 4 +- godot-core/src/builtin/vector4.rs | 4 +- godot-core/src/builtin/vector4i.rs | 4 +- godot-core/src/macros.rs | 4 +- godot-core/src/obj/gd.rs | 45 +++--- godot-core/src/obj/instance_id.rs | 2 +- godot-core/src/obj/traits.rs | 12 +- godot-ffi/src/godot_ffi.rs | 105 +++++++++----- godot-ffi/src/lib.rs | 2 +- itest/godot/ManualFfiTests.gd | 40 ------ itest/godot/TestRunner.gd | 2 - itest/rust/src/array_test.rs | 15 -- itest/rust/src/lib.rs | 7 +- itest/rust/src/node_test.rs | 4 +- itest/rust/src/object_test.rs | 4 +- itest/rust/src/runner.rs | 23 +--- itest/rust/src/variant_test.rs | 2 +- itest/rust/src/virtual_methods_test.rs | 168 +++++++++++++++++++---- 42 files changed, 339 insertions(+), 251 deletions(-) diff --git a/godot-codegen/src/central_generator.rs b/godot-codegen/src/central_generator.rs index b98383a03..2b145f68b 100644 --- a/godot-codegen/src/central_generator.rs +++ b/godot-codegen/src/central_generator.rs @@ -177,7 +177,7 @@ fn make_sys_code(central_items: &CentralItems) -> String { } // SAFETY: - // This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. + // This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for VariantType { ffi_methods! { type GDExtensionTypePtr = *mut Self; .. } } @@ -208,7 +208,7 @@ fn make_sys_code(central_items: &CentralItems) -> String { } // SAFETY: - // This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. + // This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for VariantOperator { ffi_methods! { type GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-codegen/src/lib.rs b/godot-codegen/src/lib.rs index 470af9672..e9ec07c30 100644 --- a/godot-codegen/src/lib.rs +++ b/godot-codegen/src/lib.rs @@ -258,6 +258,7 @@ const SELECTED_CLASSES: &[&str] = &[ "AudioStreamPlayer", "BaseButton", "Button", + "BoxMesh", "Camera2D", "Camera3D", "CanvasItem", @@ -276,6 +277,7 @@ const SELECTED_CLASSES: &[&str] = &[ "Label", "MainLoop", "Marker2D", + "Mesh", "Node", "Node2D", "Node3D", @@ -285,9 +287,11 @@ const SELECTED_CLASSES: &[&str] = &[ "PackedScene", "PathFollow2D", "PhysicsBody2D", + "PrimitiveMesh", "RefCounted", "RenderingServer", "Resource", + "ResourceFormatLoader", "ResourceLoader", "RigidBody2D", "SceneTree", diff --git a/godot-codegen/src/util.rs b/godot-codegen/src/util.rs index 08c8bd18f..ace6a7ea1 100644 --- a/godot-codegen/src/util.rs +++ b/godot-codegen/src/util.rs @@ -99,6 +99,8 @@ pub fn make_enum_definition(enum_: &Enum) -> TokenStream { self.ord } } + // SAFETY: + // The enums are transparently represented as an `i32`, so `*mut Self` is sound. unsafe impl sys::GodotFfi for #enum_name { sys::ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/aabb.rs b/godot-core/src/builtin/aabb.rs index e1d62767f..e6d7505bc 100644 --- a/godot-core/src/builtin/aabb.rs +++ b/godot-core/src/builtin/aabb.rs @@ -83,7 +83,7 @@ impl Aabb { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Aabb { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/array.rs b/godot-core/src/builtin/array.rs index e1bfee88e..ec0ec3ecd 100644 --- a/godot-core/src/builtin/array.rs +++ b/godot-core/src/builtin/array.rs @@ -566,22 +566,23 @@ impl Array { // ... // } +// SAFETY: +// - `move_return_ptr` +// Nothing special needs to be done beyond a `std::mem::swap` when returning an Array. +// So we can just use `ffi_methods`. +// +// - `from_arg_ptr` +// Arrays are properly initialized through a `from_sys` call, but the ref-count should be incremented +// as that is the callee's responsibility. Which we do by calling `std::mem::forget(array.share())`. unsafe impl GodotFfi for Array { ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; fn from_sys; fn sys; fn from_sys_init; - // SAFETY: - // Nothing special needs to be done beyond a `std::mem::swap` when returning an Array. fn move_return_ptr; } - // SAFETY: - // Arrays are properly initialized through a `from_sys` call, but the ref-count should be - // incremented as that is the callee's responsibility. - // - // Using `std::mem::forget(array.share())` increments the ref count. - unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::CallType) -> Self { + unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self { let array = Self::from_sys(ptr); std::mem::forget(array.share()); array diff --git a/godot-core/src/builtin/basis.rs b/godot-core/src/builtin/basis.rs index bcf204c9a..5c2d15353 100644 --- a/godot-core/src/builtin/basis.rs +++ b/godot-core/src/builtin/basis.rs @@ -571,7 +571,7 @@ impl Mul for Basis { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Basis { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/color.rs b/godot-core/src/builtin/color.rs index 5714378df..c93d97489 100644 --- a/godot-core/src/builtin/color.rs +++ b/godot-core/src/builtin/color.rs @@ -312,7 +312,7 @@ impl Color { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Color { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/dictionary.rs b/godot-core/src/builtin/dictionary.rs index bf1845cee..57a6cec56 100644 --- a/godot-core/src/builtin/dictionary.rs +++ b/godot-core/src/builtin/dictionary.rs @@ -239,22 +239,24 @@ impl Dictionary { // ---------------------------------------------------------------------------------------------------------------------------------------------- // Traits +// SAFETY: +// - `move_return_ptr` +// Nothing special needs to be done beyond a `std::mem::swap` when returning an Dictionary. +// So we can just use `ffi_methods`. +// +// - `from_arg_ptr` +// Dictionaries are properly initialized through a `from_sys` call, but the ref-count should be +// incremented as that is the callee's responsibility. Which we do by calling +// `std::mem::forget(dictionary.share())`. unsafe impl GodotFfi for Dictionary { ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; fn from_sys; fn from_sys_init; fn sys; - // SAFETY: - // Nothing special needs to be done beyond a `std::mem::swap` when returning a dictionary. fn move_return_ptr; } - // SAFETY: - // Dictionaries are properly initialized through a `from_sys` call, but the ref-count should be - // incremented as that is the callee's responsibility. - // - // Using `std::mem::forget(dictionary.share())` increments the ref count. - unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::CallType) -> Self { + unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self { let dictionary = Self::from_sys(ptr); std::mem::forget(dictionary.share()); dictionary diff --git a/godot-core/src/builtin/meta/signature.rs b/godot-core/src/builtin/meta/signature.rs index f7c25d7a6..bb219438b 100644 --- a/godot-core/src/builtin/meta/signature.rs +++ b/godot-core/src/builtin/meta/signature.rs @@ -34,7 +34,7 @@ pub trait SignatureTuple { ret: sys::GDExtensionTypePtr, func: fn(&mut C, Self::Params) -> Self::Ret, method_name: &str, - call_type: sys::CallType, + call_type: sys::PtrcallType, ); } @@ -144,7 +144,7 @@ macro_rules! impl_signature_for_tuple { ret: sys::GDExtensionTypePtr, func: fn(&mut C, Self::Params) -> Self::Ret, method_name: &str, - call_type: sys::CallType, + call_type: sys::PtrcallType, ) { $crate::out!("ptrcall: {}", method_name); diff --git a/godot-core/src/builtin/node_path.rs b/godot-core/src/builtin/node_path.rs index c6c032094..fb0b7a759 100644 --- a/godot-core/src/builtin/node_path.rs +++ b/godot-core/src/builtin/node_path.rs @@ -22,22 +22,24 @@ impl NodePath { } } +// SAFETY: +// - `move_return_ptr` +// Nothing special needs to be done beyond a `std::mem::swap` when returning a NodePath. +// So we can just use `ffi_methods`. +// +// - `from_arg_ptr` +// NodePaths are properly initialized through a `from_sys` call, but the ref-count should be +// incremented as that is the callee's responsibility. Which we do by calling +// `std::mem::forget(node_path.share())`. unsafe impl GodotFfi for NodePath { ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; fn from_sys; fn sys; fn from_sys_init; - // SAFETY: - // Nothing special needs to be done beyond a `std::mem::swap` when returning a NodePath. fn move_return_ptr; } - // SAFETY: - // NodePaths are properly initialized through a `from_sys` call, but the ref-count should be - // incremented as that is the callee's responsibility. - // - // Using `std::mem::forget(node_path.share())` increments the ref count. - unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::CallType) -> Self { + unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self { let node_path = Self::from_sys(ptr); std::mem::forget(node_path.clone()); node_path diff --git a/godot-core/src/builtin/packed_array.rs b/godot-core/src/builtin/packed_array.rs index 54eb4850b..0ba83a5b1 100644 --- a/godot-core/src/builtin/packed_array.rs +++ b/godot-core/src/builtin/packed_array.rs @@ -405,7 +405,7 @@ macro_rules! impl_packed_array { // incremented as that is the callee's responsibility. // // Using `std::mem::forget(array.clone())` increments the ref count. - unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::CallType) -> Self { + unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self { let array = Self::from_sys(ptr); std::mem::forget(array.clone()); array diff --git a/godot-core/src/builtin/plane.rs b/godot-core/src/builtin/plane.rs index c54e16b35..83209fa4d 100644 --- a/godot-core/src/builtin/plane.rs +++ b/godot-core/src/builtin/plane.rs @@ -135,7 +135,7 @@ impl Neg for Plane { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Plane { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/projection.rs b/godot-core/src/builtin/projection.rs index 81260f779..8625152e0 100644 --- a/godot-core/src/builtin/projection.rs +++ b/godot-core/src/builtin/projection.rs @@ -487,7 +487,7 @@ impl GlamConv for Projection { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Projection { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/quaternion.rs b/godot-core/src/builtin/quaternion.rs index 98b76b548..1a1f34884 100644 --- a/godot-core/src/builtin/quaternion.rs +++ b/godot-core/src/builtin/quaternion.rs @@ -255,7 +255,7 @@ impl Mul for Quaternion { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Quaternion { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/rect2.rs b/godot-core/src/builtin/rect2.rs index 82828c613..492a86e1a 100644 --- a/godot-core/src/builtin/rect2.rs +++ b/godot-core/src/builtin/rect2.rs @@ -105,7 +105,7 @@ impl Rect2 { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Rect2 { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/rect2i.rs b/godot-core/src/builtin/rect2i.rs index 4945c49a8..ea560e796 100644 --- a/godot-core/src/builtin/rect2i.rs +++ b/godot-core/src/builtin/rect2i.rs @@ -94,7 +94,7 @@ impl Rect2i { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Rect2i { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/rid.rs b/godot-core/src/builtin/rid.rs index 7dbd99fee..57eeaf394 100644 --- a/godot-core/src/builtin/rid.rs +++ b/godot-core/src/builtin/rid.rs @@ -82,7 +82,7 @@ impl Rid { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Rid { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/string.rs b/godot-core/src/builtin/string.rs index 498f48f71..99050a4d5 100644 --- a/godot-core/src/builtin/string.rs +++ b/godot-core/src/builtin/string.rs @@ -37,12 +37,12 @@ impl GodotString { fn string_sys = sys; } - /// Move `self` into a system pointer. + /// Move `self` into a system pointer. This transfers ownership and thus does not call the destructor. /// /// # Safety /// `dst` must be a pointer to a `GodotString` which is suitable for ffi with Godot. pub unsafe fn move_string_ptr(self, dst: sys::GDExtensionStringPtr) { - self.move_return_ptr(dst as *mut _, sys::CallType::Standard); + self.move_return_ptr(dst as *mut _, sys::PtrcallType::Standard); } /// Gets the internal chars slice from a [`GodotString`]. @@ -75,22 +75,24 @@ impl GodotString { } } +// SAFETY: +// - `move_return_ptr` +// Nothing special needs to be done beyond a `std::mem::swap` when returning a String. +// So we can just use `ffi_methods`. +// +// - `from_arg_ptr` +// Strings are properly initialized through a `from_sys` call, but the ref-count should be +// incremented as that is the callee's responsibility. Which we do by calling +// `std::mem::forget(string.share())`. unsafe impl GodotFfi for GodotString { ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; fn from_sys; fn sys; fn from_sys_init; - // SAFETY: - // Nothing special needs to be done beyond a `std::mem::swap` when returning a GodotString. fn move_return_ptr; } - // SAFETY: - // GodotStrings are properly initialized through a `from_sys` call, but the ref-count should be - // incremented as that is the callee's responsibility. - // - // Using `std::mem::forget(string.share())` increments the ref count. - unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::CallType) -> Self { + unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self { let string = Self::from_sys(ptr); std::mem::forget(string.clone()); string diff --git a/godot-core/src/builtin/string_name.rs b/godot-core/src/builtin/string_name.rs index 43b75fc3f..bb78daa8b 100644 --- a/godot-core/src/builtin/string_name.rs +++ b/godot-core/src/builtin/string_name.rs @@ -31,22 +31,24 @@ impl StringName { } } +// SAFETY: +// - `move_return_ptr` +// Nothing special needs to be done beyond a `std::mem::swap` when returning a StringName. +// So we can just use `ffi_methods`. +// +// - `from_arg_ptr` +// StringNames are properly initialized through a `from_sys` call, but the ref-count should be +// incremented as that is the callee's responsibility. Which we do by calling +// `std::mem::forget(string_name.share())`. unsafe impl GodotFfi for StringName { ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; fn from_sys; fn sys; fn from_sys_init; - // SAFETY: - // Nothing special needs to be done beyond a `std::mem::swap` when returning a StringName. fn move_return_ptr; } - // SAFETY: - // StringNames are properly initialized through a `from_sys` call, but the ref-count should be - // incremented as that is the callee's responsibility. - // - // Using `std::mem::forget(string_name.share())` increments the ref count. - unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::CallType) -> Self { + unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self { let string_name = Self::from_sys(ptr); std::mem::forget(string_name.clone()); string_name diff --git a/godot-core/src/builtin/transform2d.rs b/godot-core/src/builtin/transform2d.rs index 1d7ffe36d..b6b04d7d5 100644 --- a/godot-core/src/builtin/transform2d.rs +++ b/godot-core/src/builtin/transform2d.rs @@ -359,7 +359,7 @@ impl GlamConv for Transform2D { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Transform2D { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/transform3d.rs b/godot-core/src/builtin/transform3d.rs index f2459e4b6..4c7c61985 100644 --- a/godot-core/src/builtin/transform3d.rs +++ b/godot-core/src/builtin/transform3d.rs @@ -353,7 +353,7 @@ impl GlamConv for Transform3D { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Transform3D { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/vector2.rs b/godot-core/src/builtin/vector2.rs index 310f3d15b..850a9679b 100644 --- a/godot-core/src/builtin/vector2.rs +++ b/godot-core/src/builtin/vector2.rs @@ -311,7 +311,7 @@ impl_vector_operators!(Vector2, real, (x, y)); impl_vector_index!(Vector2, real, (x, y), Vector2Axis, (X, Y)); // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Vector2 { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } @@ -327,7 +327,7 @@ pub enum Vector2Axis { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Vector2Axis { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/vector2i.rs b/godot-core/src/builtin/vector2i.rs index b1109e7c5..83ec4563f 100644 --- a/godot-core/src/builtin/vector2i.rs +++ b/godot-core/src/builtin/vector2i.rs @@ -89,7 +89,7 @@ impl_vector_operators!(Vector2i, i32, (x, y)); impl_vector_index!(Vector2i, i32, (x, y), Vector2iAxis, (X, Y)); // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Vector2i { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } @@ -105,7 +105,7 @@ pub enum Vector2iAxis { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Vector2iAxis { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/vector3.rs b/godot-core/src/builtin/vector3.rs index cf83337c5..a7569bc2e 100644 --- a/godot-core/src/builtin/vector3.rs +++ b/godot-core/src/builtin/vector3.rs @@ -330,7 +330,7 @@ impl_vector_operators!(Vector3, real, (x, y, z)); impl_vector_index!(Vector3, real, (x, y, z), Vector3Axis, (X, Y, Z)); // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Vector3 { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } @@ -349,7 +349,7 @@ pub enum Vector3Axis { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Vector3Axis { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/vector3i.rs b/godot-core/src/builtin/vector3i.rs index 707f20bb4..6ae8048e4 100644 --- a/godot-core/src/builtin/vector3i.rs +++ b/godot-core/src/builtin/vector3i.rs @@ -98,7 +98,7 @@ impl_vector_operators!(Vector3i, i32, (x, y, z)); impl_vector_index!(Vector3i, i32, (x, y, z), Vector3iAxis, (X, Y, Z)); // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Vector3i { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } @@ -116,7 +116,7 @@ pub enum Vector3iAxis { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Vector3iAxis { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/vector4.rs b/godot-core/src/builtin/vector4.rs index de9f195a7..65758808d 100644 --- a/godot-core/src/builtin/vector4.rs +++ b/godot-core/src/builtin/vector4.rs @@ -90,7 +90,7 @@ impl fmt::Display for Vector4 { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Vector4 { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } @@ -110,7 +110,7 @@ pub enum Vector4Axis { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Vector4Axis { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/builtin/vector4i.rs b/godot-core/src/builtin/vector4i.rs index 7e3bfb5b5..494aca9b8 100644 --- a/godot-core/src/builtin/vector4i.rs +++ b/godot-core/src/builtin/vector4i.rs @@ -83,7 +83,7 @@ impl fmt::Display for Vector4i { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Vector4i { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } @@ -103,7 +103,7 @@ pub enum Vector4iAxis { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for Vector4iAxis { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/macros.rs b/godot-core/src/macros.rs index 1c7464f0a..5daf041a1 100644 --- a/godot-core/src/macros.rs +++ b/godot-core/src/macros.rs @@ -112,7 +112,7 @@ macro_rules! gdext_register_method_inner { inst.$method_name( $( $param, )* ) }, stringify!($method_name), - sys::CallType::Standard + sys::PtrcallType::Standard ); } ); @@ -401,7 +401,7 @@ macro_rules! gdext_ptrcall { $ret_ptr, |__instance, ($($arg,)*)| __instance.$method_name($($arg,)*), stringify!($method_name), - sys::CallType::Virtual, + sys::PtrcallType::Virtual, ) }; } diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 3b7ec0882..e001b1b08 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -12,7 +12,7 @@ use std::ptr; use godot_ffi as sys; use godot_ffi::VariantType; use sys::types::OpaqueObject; -use sys::{ffi_methods, interface_fn, static_assert_eq_size, CallType, GodotFfi}; +use sys::{ffi_methods, interface_fn, static_assert_eq_size, GodotFfi, PtrcallType}; use crate::builtin::meta::{ClassName, VariantMetadata}; use crate::builtin::{FromVariant, ToVariant, Variant, VariantConversionError}; @@ -499,7 +499,14 @@ where unsafe { std::mem::transmute::<&mut OpaqueObject, &mut T>(&mut self.opaque) } } } - +// SAFETY: +// - `move_return_ptr` +// When the `call_type` is `PtrcallType::Virtual`, and the current type is known to inherit from `RefCounted` +// then we use `ref_get_object`. Otherwise we use `Gd::from_obj_sys`. +// - `from_arg_ptr` +// When the `call_type` is `PtrcallType::Virtual`, and the current type is known to inherit from `RefCounted` +// then we use `ref_set_object`. Otherwise we use `std::ptr::write`. Finally we forget `self` as we pass +// ownership to the caller. unsafe impl GodotFfi for Gd where T: GodotClass, @@ -513,31 +520,21 @@ where // For more context around `ref_get_object` and `ref_set_object`, see: // https://github.com/godotengine/godot-cpp/issues/954 - // SAFETY: - // When the `call_type` is `CallType::Virtual`, and the current type is known to inherit from `RefCounted` - // then we use `ref_get_object`. Otherwise we use `Gd::from_obj_sys`. - unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, call_type: CallType) -> Self { - match (T::Mem::is_static_ref_counted(), call_type) { - (true, CallType::Virtual) => { - let obj_ptr = interface_fn!(ref_get_object)(ptr as sys::GDExtensionRefPtr); - // ref_get_object increments the ref_count for us - Self::from_obj_sys_weak(obj_ptr) - } - _ => Self::from_obj_sys(ptr as sys::GDExtensionObjectPtr), + unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, call_type: PtrcallType) -> Self { + if T::Mem::pass_as_ref(call_type) { + let obj_ptr = interface_fn!(ref_get_object)(ptr as sys::GDExtensionRefPtr); + // ref_get_object increments the ref_count for us + Self::from_obj_sys_weak(obj_ptr) + } else { + Self::from_obj_sys(ptr as sys::GDExtensionObjectPtr) } } - // SAFETY: - // When the `call_type` is `CallType::Virtual`, and the current type is known to inherit from `RefCounted` - // then we use `ref_set_object`. Otherwise we use `std::ptr::write`. Finally we forget `self` as we pass - // ownership to the caller. - unsafe fn move_return_ptr(self, ptr: sys::GDExtensionTypePtr, call_type: CallType) { - match (T::Mem::is_static_ref_counted(), call_type) { - (true, CallType::Virtual) => { - interface_fn!(ref_set_object)(ptr as sys::GDExtensionRefPtr, self.obj_sys()) - } - - _ => std::ptr::write(ptr as *mut _, self.opaque), + unsafe fn move_return_ptr(self, ptr: sys::GDExtensionTypePtr, call_type: PtrcallType) { + if T::Mem::pass_as_ref(call_type) { + interface_fn!(ref_set_object)(ptr as sys::GDExtensionRefPtr, self.obj_sys()) + } else { + std::ptr::write(ptr as *mut _, self.opaque) } // We've passed ownership to caller. std::mem::forget(self); diff --git a/godot-core/src/obj/instance_id.rs b/godot-core/src/obj/instance_id.rs index 4144bdbb8..b169c7070 100644 --- a/godot-core/src/obj/instance_id.rs +++ b/godot-core/src/obj/instance_id.rs @@ -78,7 +78,7 @@ impl Debug for InstanceId { } // SAFETY: -// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. +// This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for InstanceId { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } diff --git a/godot-core/src/obj/traits.rs b/godot-core/src/obj/traits.rs index abb1c0cba..b0f36508a 100644 --- a/godot-core/src/obj/traits.rs +++ b/godot-core/src/obj/traits.rs @@ -229,6 +229,8 @@ pub mod dom { // ---------------------------------------------------------------------------------------------------------------------------------------------- pub mod mem { + use godot_ffi::PtrcallType; + use super::private::Sealed; use crate::obj::{Gd, GodotClass}; use crate::out; @@ -246,7 +248,11 @@ pub mod mem { /// Check if ref-counted, return `None` if information is not available (dynamic and obj dead) fn is_ref_counted(obj: &Gd) -> Option; - fn is_static_ref_counted() -> bool { + /// Returns `true` if argument and return pointers are passed as `Ref` pointers given this + /// [`PtrcallType`]. + /// + /// See [`PtrcallType::Virtual`] for information about `Ref` objects. + fn pass_as_ref(_call_type: PtrcallType) -> bool { false } } @@ -286,8 +292,8 @@ pub mod mem { Some(true) } - fn is_static_ref_counted() -> bool { - true + fn pass_as_ref(call_type: PtrcallType) -> bool { + matches!(call_type, PtrcallType::Virtual) } } diff --git a/godot-ffi/src/godot_ffi.rs b/godot-ffi/src/godot_ffi.rs index dbb8778d6..127bc2af6 100644 --- a/godot-ffi/src/godot_ffi.rs +++ b/godot-ffi/src/godot_ffi.rs @@ -13,7 +13,7 @@ use std::fmt::Debug; /// # Safety /// /// [`from_arg_ptr`](GodotFfi::from_arg_ptr) and [`move_return_ptr`](GodotFfi::move_return_ptr) -/// must properly initialize and clean up values given the [`CallType`] provided by the caller. +/// must properly initialize and clean up values given the [`PtrcallType`] provided by the caller. pub unsafe trait GodotFfi { /// Construct from Godot opaque pointer. /// @@ -77,24 +77,26 @@ pub unsafe trait GodotFfi { /// Construct from a pointer to an argument in a call. /// /// # Safety - /// `ptr` must be a valid _type ptr_: it must follow Godot's convention to encode `Self`, - /// which is different depending on the type. - /// `ptr` must encode `Self` according to the given `call_type`'s encoding of argument values. - unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, call_type: CallType) -> Self; + /// * `ptr` must be a valid _type ptr_: it must follow Godot's convention to encode `Self`, + /// which is different depending on the type. + /// + /// * `ptr` must encode `Self` according to the given `call_type`'s encoding of argument values. + unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, call_type: PtrcallType) -> Self; /// Move self into the pointer in pointer `dst`, dropping what is already in `dst. /// /// # Safety - /// `dst` must be a valid _type ptr_: it must follow Godot's convention to encode `Self`, - /// hich is different depending on the type. - /// `dst` must be able to accept a value of type `Self` encoded according to the given - /// `call_type`'s encoding of return values. - unsafe fn move_return_ptr(self, dst: sys::GDExtensionTypePtr, call_type: CallType); + /// * `dst` must be a valid _type ptr_: it must follow Godot's convention to encode `Self`, + /// which is different depending on the type. + /// + /// * `dst` must be able to accept a value of type `Self` encoded according to the given + /// `call_type`'s encoding of return values. + unsafe fn move_return_ptr(self, dst: sys::GDExtensionTypePtr, call_type: PtrcallType); } /// An indication of what type of pointer call is being made. #[derive(Default, Copy, Clone, Eq, PartialEq, Debug)] -pub enum CallType { +pub enum PtrcallType { /// Standard pointer call. /// /// In a standard ptrcall, every argument is passed in as a pointer to a value of that type, and the @@ -103,8 +105,8 @@ pub enum CallType { Standard, /// Virtual pointer call. /// - /// A virtual call behaves like [`CallType::Standard`], except for `RefCounted` objects. - /// `RefCounted` objects are instead passed in and returned as `Ref` objects. + /// A virtual call behaves like [`PtrcallType::Standard`], except for `RefCounted` objects. + /// `RefCounted` objects are instead passed in and returned as `Ref` objects in Godot. /// /// To properly get a value from an argument in a pointer call, you must use `ref_get_object`. And to /// return a value you must use `ref_set_object`. @@ -126,7 +128,7 @@ pub trait GodotFuncMarshal: Sized { /// See also [`GodotFfi::from_arg_ptr`]. unsafe fn try_from_arg( ptr: sys::GDExtensionTypePtr, - call_type: CallType, + call_type: PtrcallType, ) -> Result; /// Used for function return values. On failure, `self` which can't be converted to Via is returned. @@ -138,7 +140,7 @@ pub trait GodotFuncMarshal: Sized { unsafe fn try_return( self, dst: sys::GDExtensionTypePtr, - call_type: CallType, + call_type: PtrcallType, ) -> Result<(), Self>; } @@ -175,13 +177,13 @@ macro_rules! ffi_methods_one { }; (OpaquePtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $from_arg_ptr:ident = from_arg_ptr) => { $( #[$attr] )? $vis - unsafe fn $from_arg_ptr(ptr: $Ptr, _call_type: $crate::CallType) -> Self { + unsafe fn $from_arg_ptr(ptr: $Ptr, _call_type: $crate::PtrcallType) -> Self { Self::from_sys(ptr as *mut _) } }; (OpaquePtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $move_return_ptr:ident = move_return_ptr) => { $( #[$attr] )? $vis - unsafe fn $move_return_ptr(mut self, dst: $Ptr, _call_type: $crate::CallType) { + unsafe fn $move_return_ptr(mut self, dst: $Ptr, _call_type: $crate::PtrcallType) { std::ptr::swap(dst as *mut _, std::ptr::addr_of_mut!(self.opaque)) } }; @@ -210,13 +212,13 @@ macro_rules! ffi_methods_one { }; (OpaqueValue $Ptr:ty; $( #[$attr:meta] )? $vis:vis $from_arg_ptr:ident = from_arg_ptr) => { $( #[$attr] )? $vis - unsafe fn $from_arg_ptr(ptr: $Ptr, _call_type: $crate::CallType) -> Self { + unsafe fn $from_arg_ptr(ptr: $Ptr, _call_type: $crate::PtrcallType) -> Self { Self::from_sys(ptr as *mut _) } }; (OpaqueValue $Ptr:ty; $( #[$attr:meta] )? $vis:vis $move_return_ptr:ident = move_return_ptr) => { $( #[$attr] )? $vis - unsafe fn $move_return_ptr(mut self, dst: $Ptr, _call_type: $crate::CallType) { + unsafe fn $move_return_ptr(mut self, dst: $Ptr, _call_type: $crate::PtrcallType) { std::ptr::swap(dst, std::mem::transmute::<_, $Ptr>(self.opaque)) } }; @@ -245,13 +247,13 @@ macro_rules! ffi_methods_one { }; (SelfPtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $from_arg_ptr:ident = from_arg_ptr) => { $( #[$attr] )? $vis - unsafe fn $from_arg_ptr(ptr: $Ptr, _call_type: $crate::CallType) -> Self { + unsafe fn $from_arg_ptr(ptr: $Ptr, _call_type: $crate::PtrcallType) -> Self { *(ptr as *mut Self) } }; (SelfPtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $move_return_ptr:ident = move_return_ptr) => { $( #[$attr] )? $vis - unsafe fn $move_return_ptr(self, dst: $Ptr, _call_type: $crate::CallType) { + unsafe fn $move_return_ptr(self, dst: $Ptr, _call_type: $crate::PtrcallType) { *(dst as *mut Self) = self } }; @@ -313,7 +315,7 @@ macro_rules! ffi_methods_rest { /// /// ## Using `Opaque` /// -/// Turning pointer call arguments into a value is simply calling `from_opaue` on the argument pointer. +/// Turning pointer call arguments into a value is simply calling `from_opaque` on the argument pointer. /// Returning a value from a pointer call is simply calling [`std::ptr::swap`] on the return pointer /// and the `opaque` field transmuted into a pointer. /// @@ -348,14 +350,14 @@ macro_rules! ffi_methods { // ---------------------------------------------------------------------------------------------------------------------------------------------- // Implementation for common types (needs to be this crate due to orphan rule) mod scalars { - use super::{CallType, GodotFfi, GodotFuncMarshal}; + use super::{GodotFfi, GodotFuncMarshal, PtrcallType}; use crate as sys; use std::convert::Infallible; macro_rules! impl_godot_marshalling { ($T:ty) => { // SAFETY: - // This type is transparently represented as `Self` in Godot, so `*mut Self` is sound. + // This type is represented as `Self` in Godot, so `*mut Self` is sound. unsafe impl GodotFfi for $T { ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. } } @@ -370,21 +372,44 @@ mod scalars { unsafe fn try_from_arg( ptr: sys::GDExtensionTypePtr, - call_type: CallType, + call_type: PtrcallType, ) -> Result { - let via = <$Via as GodotFfi>::from_arg_ptr(ptr, call_type); - // Godot does not always give us a valid `i64`/`f64`, nor `Self` when it does a pointer - // call, so we cannot use `try_from` here. + let via = <$Via>::from_arg_ptr(ptr, call_type); + Self::try_from(via).map_err(|_| via) + } + + unsafe fn try_return( + self, + dst: sys::GDExtensionTypePtr, + call_type: PtrcallType, + ) -> Result<(), Self> { + <$Via>::from(self).move_return_ptr(dst, call_type); + Ok(()) + } + } + }; + + ($T:ty as $Via:ty; lossy) => { + // implicit bounds: + // T: TryFrom, Copy + // Via: TryFrom, GodotFfi + impl GodotFuncMarshal for $T { + type Via = $Via; + + unsafe fn try_from_arg( + ptr: sys::GDExtensionTypePtr, + call_type: PtrcallType, + ) -> Result { + let via = <$Via>::from_arg_ptr(ptr, call_type); Ok(via as Self) } unsafe fn try_return( self, dst: sys::GDExtensionTypePtr, - call_type: CallType, + call_type: PtrcallType, ) -> Result<(), Self> { - let via = <$Via>::try_from(self).map_err(|_| self)?; - via.move_return_ptr(dst, call_type); + (self as $Via).move_return_ptr(dst, call_type); Ok(()) } } @@ -398,12 +423,13 @@ mod scalars { // Only implements GodotFuncMarshal impl_godot_marshalling!(i32 as i64); - impl_godot_marshalling!(i16 as i64); - impl_godot_marshalling!(i8 as i64); impl_godot_marshalling!(u32 as i64); + impl_godot_marshalling!(i16 as i64); impl_godot_marshalling!(u16 as i64); + impl_godot_marshalling!(i8 as i64); impl_godot_marshalling!(u8 as i64); - impl_godot_marshalling!(f32 as f64); + + impl_godot_marshalling!(f32 as f64; lossy); unsafe impl GodotFfi for () { unsafe fn from_sys(_ptr: sys::GDExtensionTypePtr) -> Self { @@ -421,7 +447,10 @@ mod scalars { // SAFETY: // We're not accessing the value in `_ptr`. - unsafe fn from_arg_ptr(_ptr: sys::GDExtensionTypePtr, _call_type: super::CallType) -> Self { + unsafe fn from_arg_ptr( + _ptr: sys::GDExtensionTypePtr, + _call_type: super::PtrcallType, + ) -> Self { } // SAFETY: @@ -429,7 +458,7 @@ mod scalars { unsafe fn move_return_ptr( self, _dst: sys::GDExtensionTypePtr, - _call_type: super::CallType, + _call_type: super::PtrcallType, ) { // Do nothing } @@ -443,7 +472,7 @@ mod scalars { unsafe fn try_from_arg( ptr: sys::GDExtensionTypePtr, - call_type: CallType, + call_type: PtrcallType, ) -> Result { Ok(Self::from_arg_ptr(ptr, call_type)) } @@ -451,7 +480,7 @@ mod scalars { unsafe fn try_return( self, dst: sys::GDExtensionTypePtr, - call_type: CallType, + call_type: PtrcallType, ) -> Result<(), Self> { self.move_return_ptr(dst, call_type); Ok(()) diff --git a/godot-ffi/src/lib.rs b/godot-ffi/src/lib.rs index a4a07478f..fa94018cb 100644 --- a/godot-ffi/src/lib.rs +++ b/godot-ffi/src/lib.rs @@ -28,7 +28,7 @@ mod plugins; #[doc(hidden)] pub use paste; -pub use crate::godot_ffi::{CallType, GodotFfi, GodotFuncMarshal}; +pub use crate::godot_ffi::{GodotFfi, GodotFuncMarshal, PtrcallType}; pub use gen::central::*; pub use gen::gdextension_interface::*; diff --git a/itest/godot/ManualFfiTests.gd b/itest/godot/ManualFfiTests.gd index 4b184c72c..73b56f8f9 100644 --- a/itest/godot/ManualFfiTests.gd +++ b/itest/godot/ManualFfiTests.gd @@ -72,46 +72,6 @@ func test_export(): obj.free() node.free() -func test_untyped_array_pass_to_user_func_varcall(): - var obj = ArrayTest.new() - var array: Array = [42, "answer"] - assert_eq(obj.pass_untyped_array(array), 2) - -func test_untyped_array_pass_to_user_func_ptrcall(): - var obj: ArrayTest = ArrayTest.new() - var array: Array = [42, "answer"] - assert_eq(obj.pass_untyped_array(array), 2) - -func test_untyped_array_return_from_user_func_varcall(): - var obj = ArrayTest.new() - var array: Array = obj.return_untyped_array() - assert_eq(array, [42, "answer"]) - -func test_untyped_array_return_from_user_func_ptrcall(): - var obj: ArrayTest = ArrayTest.new() - var array: Array = obj.return_untyped_array() - assert_eq(array, [42, "answer"]) - -func test_typed_array_pass_to_user_func_varcall(): - var obj = ArrayTest.new() - var array: Array[int] = [1, 2, 3] - assert_eq(obj.pass_typed_array(array), 6) - -func test_typed_array_pass_to_user_func_ptrcall(): - var obj: ArrayTest = ArrayTest.new() - var array: Array[int] = [1, 2, 3] - assert_eq(obj.pass_typed_array(array), 6) - -func test_typed_array_return_from_user_func_varcall(): - var obj = ArrayTest.new() - var array: Array[int] = obj.return_typed_array(3) - assert_eq(array, [1, 2, 3]) - -func test_typed_array_return_from_user_func_ptrcall(): - var obj: ArrayTest = ArrayTest.new() - var array: Array[int] = obj.return_typed_array(3) - assert_eq(array, [1, 2, 3]) - class MockObjGd extends Object: var i: int = 0 diff --git a/itest/godot/TestRunner.gd b/itest/godot/TestRunner.gd index 9466ecf99..3e28abe2f 100644 --- a/itest/godot/TestRunner.gd +++ b/itest/godot/TestRunner.gd @@ -38,9 +38,7 @@ func _ready(): gdscript_tests, gdscript_suites.size(), allow_focus, - get_tree().root, self, - get_tree() ) var exit_code: int = 0 if success else 1 diff --git a/itest/rust/src/array_test.rs b/itest/rust/src/array_test.rs index 9f830d327..e8ac4e4a9 100644 --- a/itest/rust/src/array_test.rs +++ b/itest/rust/src/array_test.rs @@ -427,21 +427,6 @@ struct ArrayTest; #[godot_api] impl ArrayTest { - #[func] - fn pass_untyped_array(&self, array: VariantArray) -> i64 { - array.len().try_into().unwrap() - } - - #[func] - fn return_untyped_array(&self) -> VariantArray { - varray![42, "answer"] - } - - #[func] - fn pass_typed_array(&self, array: Array) -> i64 { - array.iter_shared().sum() - } - #[func] fn return_typed_array(&self, n: i64) -> Array { (1..(n + 1)).collect() diff --git a/itest/rust/src/lib.rs b/itest/rust/src/lib.rs index 17670ef25..9596878f6 100644 --- a/itest/rust/src/lib.rs +++ b/itest/rust/src/lib.rs @@ -4,10 +4,9 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use godot::engine::{Engine, Node, Window}; +use godot::engine::{Engine, Node}; use godot::init::{gdextension, ExtensionLibrary}; use godot::obj::Gd; -use godot::prelude::SceneTree; use godot::sys; mod array_test; @@ -105,9 +104,7 @@ fn collect_rust_tests() -> (Vec, usize, bool) { } pub struct TestContext { - _scene_tree: Gd, - root_node: Gd, - test_node: Gd, + scene_tree: Gd, } #[derive(Copy, Clone)] diff --git a/itest/rust/src/node_test.rs b/itest/rust/src/node_test.rs index 2daf9189d..5099ed647 100644 --- a/itest/rust/src/node_test.rs +++ b/itest/rust/src/node_test.rs @@ -58,7 +58,7 @@ fn node_get_node_fail() { #[itest] fn node_path_from_str(ctx: &TestContext) { - let child = ctx.test_node.share(); + let child = ctx.scene_tree.share(); assert_eq!( child.get_path().to_string(), NodePath::from_str("/root/TestRunner").unwrap().to_string() @@ -96,7 +96,7 @@ fn node_scene_tree() { // FIXME: call_group() crashes #[itest(skip)] fn node_call_group(ctx: &TestContext) { - let mut node = ctx.test_node.share(); + let mut node = ctx.scene_tree.share(); let mut tree = node.get_tree().unwrap(); node.add_to_group("group".into(), true); diff --git a/itest/rust/src/object_test.rs b/itest/rust/src/object_test.rs index b1dd8de2a..4443490e3 100644 --- a/itest/rust/src/object_test.rs +++ b/itest/rust/src/object_test.rs @@ -95,7 +95,7 @@ fn object_user_roundtrip_write() { assert_eq!(obj.bind().value, value); let obj2 = unsafe { - Gd::::from_sys_init(|ptr| obj.move_return_ptr(ptr, sys::CallType::Standard)) + Gd::::from_sys_init(|ptr| obj.move_return_ptr(ptr, sys::PtrcallType::Standard)) }; assert_eq!(obj2.bind().value, value); } // drop @@ -615,7 +615,7 @@ fn object_call_with_args() { fn object_get_scene_tree(ctx: &TestContext) { let node = Node3D::new_alloc(); - let mut tree = ctx.test_node.share(); + let mut tree = ctx.scene_tree.share(); tree.add_child(node.upcast(), false, InternalMode::INTERNAL_MODE_DISABLED); let count = tree.get_child_count(false); diff --git a/itest/rust/src/runner.rs b/itest/rust/src/runner.rs index 30369b475..70c452f73 100644 --- a/itest/rust/src/runner.rs +++ b/itest/rust/src/runner.rs @@ -8,9 +8,8 @@ use std::time::{Duration, Instant}; use godot::bind::{godot_api, GodotClass}; use godot::builtin::{ToVariant, Variant, VariantArray}; -use godot::engine::{Node, Window}; +use godot::engine::Node; use godot::obj::Gd; -use godot::prelude::SceneTree; use crate::{RustTestCase, TestContext}; @@ -32,9 +31,7 @@ impl IntegrationTests { gdscript_tests: VariantArray, gdscript_file_count: i64, allow_focus: bool, - root_node: Gd, - test_node: Gd, - scene_tree: Gd, + scene_tree: Gd, ) -> bool { println!("{}Run{} Godot integration tests...", FMT_CYAN_BOLD, FMT_END); @@ -57,7 +54,7 @@ impl IntegrationTests { } let clock = Instant::now(); - self.run_rust_tests(rust_tests, root_node, test_node, scene_tree); + self.run_rust_tests(rust_tests, scene_tree); let rust_time = clock.elapsed(); let gdscript_time = if !focus_run { self.run_gdscript_tests(gdscript_tests); @@ -69,18 +66,8 @@ impl IntegrationTests { self.conclude(rust_time, gdscript_time, allow_focus) } - fn run_rust_tests( - &mut self, - tests: Vec, - root_node: Gd, - test_node: Gd, - scene_tree: Gd, - ) { - let ctx = TestContext { - _scene_tree: scene_tree, - root_node, - test_node, - }; + fn run_rust_tests(&mut self, tests: Vec, scene_tree: Gd) { + let ctx = TestContext { scene_tree }; let mut last_file = None; for test in tests { diff --git a/itest/rust/src/variant_test.rs b/itest/rust/src/variant_test.rs index 06d0e1197..9809e669a 100644 --- a/itest/rust/src/variant_test.rs +++ b/itest/rust/src/variant_test.rs @@ -257,7 +257,7 @@ fn variant_sys_conversion2() { unsafe { v.clone().move_return_ptr( buffer.as_mut_ptr() as sys::GDExtensionTypePtr, - sys::CallType::Standard, + sys::PtrcallType::Standard, ) }; diff --git a/itest/rust/src/virtual_methods_test.rs b/itest/rust/src/virtual_methods_test.rs index e17c5be60..03edf03ed 100644 --- a/itest/rust/src/virtual_methods_test.rs +++ b/itest/rust/src/virtual_methods_test.rs @@ -8,14 +8,20 @@ use crate::TestContext; use godot::bind::{godot_api, GodotClass}; -use godot::builtin::GodotString; +use godot::builtin::{ + is_equal_approx, real, varray, Color, GodotString, PackedByteArray, PackedColorArray, + PackedFloat32Array, PackedInt32Array, PackedStringArray, PackedVector2Array, + PackedVector3Array, RealConv, StringName, ToVariant, Variant, VariantArray, Vector2, Vector3, +}; use godot::engine::node::InternalMode; +use godot::engine::resource_loader::CacheMode; use godot::engine::{ - InputEvent, InputEventAction, Node, Node2D, Node2DVirtual, NodeVirtual, RefCounted, - RefCountedVirtual, + BoxMesh, InputEvent, InputEventAction, Node, Node2D, Node2DVirtual, PrimitiveMesh, + PrimitiveMeshVirtual, RefCounted, RefCountedVirtual, ResourceFormatLoader, + ResourceFormatLoaderVirtual, ResourceLoader, Window, }; use godot::obj::{Base, Gd, Share}; -use godot::prelude::PackedStringArray; +use godot::private::class_macros::assert_eq_approx; use godot::test::itest; /// Simple class, that deliberately has no constructor accessible from GDScript @@ -96,25 +102,34 @@ impl Node2DVirtual for TreeVirtualTest { } #[derive(GodotClass, Debug)] -#[class(base=Node)] +#[class(init, base=PrimitiveMesh)] struct ReturnVirtualTest { #[base] - base: Base, + base: Base, } #[godot_api] -impl NodeVirtual for ReturnVirtualTest { - fn init(base: Base) -> Self { - ReturnVirtualTest { base } - } - - fn get_configuration_warnings(&self) -> PackedStringArray { - let mut output = PackedStringArray::new(); - output.push("Hello".into()); - output +impl PrimitiveMeshVirtual for ReturnVirtualTest { + fn create_mesh_array(&self) -> VariantArray { + varray![ + PackedVector3Array::from_iter([Vector3::LEFT].into_iter()), + PackedVector3Array::from_iter([Vector3::LEFT].into_iter()), + PackedFloat32Array::from_iter([0.0, 0.0, 0.0, 1.0].into_iter()), + PackedColorArray::from_iter([Color::from_rgb(1.0, 1.0, 1.0)]), + PackedVector2Array::from_iter([Vector2::LEFT]), + PackedVector2Array::from_iter([Vector2::LEFT]), + PackedByteArray::from_iter([0, 1, 2, 3].into_iter()), + PackedByteArray::from_iter([0, 1, 2, 3].into_iter()), + PackedByteArray::from_iter([0, 1, 2, 3].into_iter()), + PackedByteArray::from_iter([0, 1, 2, 3].into_iter()), + PackedInt32Array::from_iter([0, 1, 2, 3].into_iter()), + PackedFloat32Array::from_iter([0.0, 1.0, 2.0, 3.0].into_iter()), + PackedInt32Array::from_iter([0].into_iter()), + ] } } +#[derive(GodotClass, Debug)] #[class(base=Node2D)] struct InputVirtualTest { #[base] @@ -129,11 +144,52 @@ impl Node2DVirtual for InputVirtualTest { } fn input(&mut self, event: Gd) { - println!("nya"); self.event = Some(event); } } +#[derive(GodotClass, Debug)] +#[class(init, base=ResourceFormatLoader)] +struct FormatLoaderTest { + #[base] + base: Base, +} + +impl FormatLoaderTest { + fn resource_type() -> GodotString { + GodotString::from("foo") + } +} + +#[godot_api] +impl ResourceFormatLoaderVirtual for FormatLoaderTest { + fn get_recognized_extensions(&self) -> PackedStringArray { + [GodotString::from("extension")].into_iter().collect() + } + + fn handles_type(&self, type_: StringName) -> bool { + type_.to_string() == Self::resource_type().to_string() + } + + fn get_resource_type(&self, _path: GodotString) -> GodotString { + Self::resource_type() + } + + fn exists(&self, _path: GodotString) -> bool { + true + } + + fn load( + &self, + _path: GodotString, + _original_path: GodotString, + _use_sub_threads: bool, + _cache_mode: i64, + ) -> Variant { + BoxMesh::new().to_variant() + } +} + // ---------------------------------------------------------------------------------------------------------------------------------------------- #[itest] @@ -147,7 +203,7 @@ fn test_ready(test_context: &TestContext) { assert_eq!(obj.bind().implementation_value, 0); // Add to scene tree - let mut test_node = test_context.test_node.share(); + let mut test_node = test_context.scene_tree.share(); test_node.add_child( obj.share().upcast(), false, @@ -163,7 +219,7 @@ fn test_ready_multiple_fires(test_context: &TestContext) { let obj = Gd::::new_default(); assert_eq!(obj.bind().implementation_value, 0); - let mut test_node = test_context.test_node.share(); + let mut test_node = test_context.scene_tree.share(); // Add to scene tree test_node.add_child( @@ -192,7 +248,7 @@ fn test_ready_request_ready(test_context: &TestContext) { let obj = Gd::::new_default(); assert_eq!(obj.bind().implementation_value, 0); - let mut test_node = test_context.test_node.share(); + let mut test_node = test_context.scene_tree.share(); // Add to scene tree test_node.add_child( @@ -235,7 +291,7 @@ fn test_tree_enters_exits(test_context: &TestContext) { let obj = Gd::::new_default(); assert_eq!(obj.bind().tree_enters, 0); assert_eq!(obj.bind().tree_exits, 0); - let mut test_node = test_context.test_node.share(); + let mut test_node = test_context.scene_tree.share(); // Add to scene tree test_node.add_child( @@ -262,17 +318,76 @@ fn test_tree_enters_exits(test_context: &TestContext) { #[itest] fn test_virtual_method_with_return(_test_context: &TestContext) { let obj = Gd::::new_default(); - let output = obj.bind().get_configuration_warnings(); - assert!(output.contains("Hello".into())); - assert_eq!(output.len(), 1); - obj.free(); + let arr = obj.share().upcast::().get_mesh_arrays(); + let arr_rust = obj.bind().create_mesh_array(); + assert_eq!(arr.len(), arr_rust.len()); + // can't just assert_eq because the values of some floats change slightly + assert_eq_approx!( + arr.get(0).to::().get(0), + arr_rust.get(0).to::().get(0), + Vector3::is_equal_approx + ); + assert_eq_approx!( + arr.get(2).to::().get(3), + arr_rust.get(2).to::().get(3), + |a, b| is_equal_approx(real::from_f32(a), real::from_f32(b)) + ); + assert_eq_approx!( + arr.get(3).to::().get(0), + arr_rust.get(3).to::().get(0), + Color::is_equal_approx + ); + assert_eq_approx!( + arr.get(4).to::().get(0), + arr_rust.get(4).to::().get(0), + Vector2::is_equal_approx + ); + assert_eq!( + arr.get(6).to::(), + arr_rust.get(6).to::(), + ); + assert_eq!( + arr.get(10).to::(), + arr_rust.get(10).to::() + ); } +// TODO: Fix memory leak in this test. +#[itest(skip)] +fn test_format_loader(_test_context: &TestContext) { + let format_loader = Gd::::new_default(); + let mut loader = ResourceLoader::singleton(); + loader.add_resource_format_loader(format_loader.share().upcast(), true); + + let extensions = loader.get_recognized_extensions_for_type(FormatLoaderTest::resource_type()); + let mut extensions_rust = format_loader.bind().get_recognized_extensions(); + extensions_rust.push("tres".into()); + assert_eq!(extensions, extensions_rust); + let resource = loader + .load( + "path.extension".into(), + "".into(), + CacheMode::CACHE_MODE_IGNORE, + ) + .unwrap(); + assert!(resource.try_cast::().is_some()); + + loader.remove_resource_format_loader(format_loader.upcast()); +} + +#[itest] fn test_input_event(test_context: &TestContext) { let obj = Gd::::new_default(); assert_eq!(obj.bind().event, None); + let test_viewport = Window::new_alloc(); + + test_context.scene_tree.share().add_child( + test_viewport.share().upcast(), + false, + InternalMode::INTERNAL_MODE_DISABLED, + ); - test_context.test_node.share().add_child( + test_viewport.share().add_child( obj.share().upcast(), false, InternalMode::INTERNAL_MODE_DISABLED, @@ -283,8 +398,7 @@ fn test_input_event(test_context: &TestContext) { event.set_pressed(true); // We're running in headless mode, so Input.parse_input_event does not work - test_context - .root_node + test_viewport .share() .push_input(event.share().upcast(), false);