Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

impl GodotFfi for Option<T> when T is pointer sized and nullable #240 #247

Merged
merged 1 commit into from
May 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion godot-core/src/obj/gd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ 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, PtrcallType};
use sys::{
ffi_methods, interface_fn, static_assert_eq_size, GodotFfi, GodotNullablePtr, PtrcallType,
};

use crate::builtin::meta::{ClassName, VariantMetadata};
use crate::builtin::{
Expand Down Expand Up @@ -570,6 +572,11 @@ where
}
}

// SAFETY:
// `Gd<T: GodotClass>` will only contain types that inherit from `crate::engine::Object`.
// Godots `Object` in turn is known to be nullable and always a pointer.
unsafe impl<T: GodotClass> GodotNullablePtr for Gd<T> {}

impl<T: GodotClass> Gd<T> {
/// Runs `init_fn` on the address of a pointer (initialized to null). If that pointer is still null after the `init_fn` call,
/// then `None` will be returned; otherwise `Gd::from_obj_sys(ptr)`.
Expand Down Expand Up @@ -754,6 +761,25 @@ impl<T: GodotClass> ToVariant for Gd<T> {
}
}

impl<T: GodotClass> ToVariant for Option<Gd<T>> {
fn to_variant(&self) -> Variant {
match self {
Some(gd) => gd.to_variant(),
None => Variant::nil(),
}
}
}

impl<T: GodotClass> FromVariant for Option<Gd<T>> {
fn try_from_variant(variant: &Variant) -> Result<Self, VariantConversionError> {
if variant.is_nil() {
Ok(None)
} else {
Gd::try_from_variant(variant).map(Some)
}
}
}

impl<T: GodotClass> PartialEq for Gd<T> {
/// ⚠️ Returns whether two `Gd` pointers point to the same object.
///
Expand Down
51 changes: 49 additions & 2 deletions godot-ffi/src/godot_ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
*/

use crate as sys;
use std::fmt::Debug;
use crate::{self as sys, ptr_then};
use std::{fmt::Debug, ptr};

/// Adds methods to convert from and to Godot FFI pointers.
/// See [crate::ffi_methods] for ergonomic implementation.
Expand Down Expand Up @@ -96,6 +96,53 @@ pub unsafe trait GodotFfi {
unsafe fn move_return_ptr(self, dst: sys::GDExtensionTypePtr, call_type: PtrcallType);
}

/// Marks a type as having a nullable counterpart in Godot.
///
/// This trait primarily exists to implement GodotFfi for `Option<Gd<T>>`, which is not possible
/// due to Rusts orphan rule. The rule also enforces better API design, though. `godot_ffi` should
/// not concern itself with the details of how Godot types work and merely defines the FFI abstraction.
/// By having a marker trait for nullable types, we can provide a generic implementation for
/// compatible types, without knowing their definition.
///
/// # Safety
///
/// The type has to have a pointer-sized counterpart in Godot, which needs to be nullable.
/// So far, this only applies to class types (Object hierarchy).
pub unsafe trait GodotNullablePtr: GodotFfi {}

unsafe impl<T> GodotFfi for Option<T>
where
T: GodotNullablePtr,
{
fn sys(&self) -> sys::GDExtensionTypePtr {
match self {
Some(value) => value.sys(),
None => ptr::null_mut() as sys::GDExtensionTypePtr,
}
}

unsafe fn from_sys(ptr: sys::GDExtensionTypePtr) -> Self {
ptr_then(ptr, |ptr| T::from_sys(ptr))
}
TitanNano marked this conversation as resolved.
Show resolved Hide resolved

unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionUninitializedTypePtr)) -> Self {
let mut raw = std::mem::MaybeUninit::uninit();
init_fn(raw.as_mut_ptr() as sys::GDExtensionUninitializedTypePtr);

Self::from_sys(raw.assume_init())
}

unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, call_type: PtrcallType) -> Self {
ptr_then(ptr, |ptr| T::from_arg_ptr(ptr, call_type))
}

unsafe fn move_return_ptr(self, ptr: sys::GDExtensionTypePtr, call_type: PtrcallType) {
if let Some(value) = self {
value.move_return_ptr(ptr, call_type)
}
}
}

/// An indication of what type of pointer call is being made.
#[derive(Default, Copy, Clone, Eq, PartialEq, Debug)]
pub enum PtrcallType {
Expand Down
2 changes: 1 addition & 1 deletion godot-ffi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ use std::ffi::CStr;
#[doc(hidden)]
pub use paste;

pub use crate::godot_ffi::{GodotFfi, GodotFuncMarshal, PtrcallType};
pub use crate::godot_ffi::{GodotFfi, GodotFuncMarshal, GodotNullablePtr, PtrcallType};
pub use gen::central::*;
pub use gen::gdextension_interface::*;
pub use gen::interface::*;
Expand Down
86 changes: 85 additions & 1 deletion itest/godot/ManualFfiTests.gd
Original file line number Diff line number Diff line change
Expand Up @@ -149,4 +149,88 @@ func test_refcounted_as_object_return_from_user_func_ptrcall():
func test_custom_constructor():
var obj = CustomConstructor.construct_object(42)
assert_eq(obj.val, 42)
obj.free()
obj.free()

func test_option_refcounted_none_varcall():
var ffi := OptionFfiTest.new()

var from_rust: Variant = ffi.return_option_refcounted_none()
assert_that(ffi.accept_option_refcounted_none(from_rust), "ffi.accept_option_refcounted_none(from_rust)")

var from_gdscript: Variant = null
var mirrored: Variant = ffi.mirror_option_refcounted(from_gdscript)
assert_eq(mirrored, from_gdscript, "mirrored == from_gdscript")

func test_option_refcounted_none_ptrcall():
var ffi := OptionFfiTest.new()

var from_rust: Object = ffi.return_option_refcounted_none()
assert_that(ffi.accept_option_refcounted_none(from_rust), "ffi.accept_option_refcounted_none(from_rust)")

var from_gdscript: Object = null
var mirrored: Object = ffi.mirror_option_refcounted(from_gdscript)
assert_eq(mirrored, from_gdscript, "mirrored == from_gdscript")

func test_option_refcounted_some_varcall():
var ffi := OptionFfiTest.new()

var from_rust: Variant = ffi.return_option_refcounted_some()
assert_that(ffi.accept_option_refcounted_some(from_rust), "ffi.accept_option_refcounted_some(from_rust)")

var from_gdscript: Variant = RefCounted.new()
var mirrored: Variant = ffi.mirror_option_refcounted(from_gdscript)
assert_eq(mirrored, from_gdscript, "mirrored == from_gdscript")

func test_option_refcounted_some_ptrcall():
var ffi := OptionFfiTest.new()

var from_rust: Object = ffi.return_option_refcounted_some()
assert_that(ffi.accept_option_refcounted_some(from_rust), "ffi.accept_option_refcounted_some(from_rust)")

var from_gdscript: Object = RefCounted.new()
var mirrored: Object = ffi.mirror_option_refcounted(from_gdscript)
assert_eq(mirrored, from_gdscript, "mirrored == from_gdscript")

func test_option_node_none_varcall():
var ffi := OptionFfiTest.new()

var from_rust: Variant = ffi.return_option_node_none()
assert_that(ffi.accept_option_node_none(from_rust), "ffi.accept_option_node_none(from_rust)")

var from_gdscript: Variant = null
var mirrored: Variant = ffi.mirror_option_node(from_gdscript)
assert_eq(mirrored, from_gdscript, "mirrored == from_gdscript")

func test_option_node_none_ptrcall():
var ffi := OptionFfiTest.new()

var from_rust: Node = ffi.return_option_node_none()
assert_that(ffi.accept_option_node_none(from_rust), "ffi.accept_option_node_none(from_rust)")

var from_gdscript: Node = null
var mirrored: Node = ffi.mirror_option_node(from_gdscript)
assert_eq(mirrored, from_gdscript, "mirrored == from_gdscript")

func test_option_node_some_varcall():
var ffi := OptionFfiTest.new()

var from_rust: Variant = ffi.return_option_node_some()
assert_that(ffi.accept_option_node_some(from_rust), "ffi.accept_option_node_some(from_rust)")

var from_gdscript: Variant = Node.new()
var mirrored: Variant = ffi.mirror_option_node(from_gdscript)
assert_eq(mirrored, from_gdscript, "mirrored == from_gdscript")
from_gdscript.free()
from_rust.free()

func test_option_node_some_ptrcall():
var ffi := OptionFfiTest.new()

var from_rust: Node = ffi.return_option_node_some()
assert_that(ffi.accept_option_node_some(from_rust), "ffi.accept_option_node_some(from_rust)")

var from_gdscript: Node = Node.new()
var mirrored: Node = ffi.mirror_option_node(from_gdscript)
assert_eq(mirrored, from_gdscript, "mirrored == from_gdscript")
from_gdscript.free()
from_rust.free()
1 change: 1 addition & 0 deletions itest/rust/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ mod init_test;
mod native_structures_test;
mod node_test;
mod object_test;
mod option_ffi_test;
mod packed_array_test;
mod projection_test;
mod quaternion_test;
Expand Down
87 changes: 87 additions & 0 deletions itest/rust/src/option_ffi_test.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/*
* 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/.
*/

use godot::prelude::{godot_api, Gd, GodotClass, Node, Object, RefCounted};
use godot::sys::GodotFfi;

use crate::itest;

#[itest]
fn option_some_sys_conversion() {
let v = Some(Object::new_alloc());
let ptr = v.sys();

let v2 = unsafe { Option::<Gd<Object>>::from_sys(ptr) };
assert_eq!(v2, v);

v.unwrap().free();
}

#[itest]
fn option_none_sys_conversion() {
let v = None;
let ptr = v.sys();

let v2 = unsafe { Option::<Gd<Object>>::from_sys(ptr) };
assert_eq!(v2, v);
}

#[derive(GodotClass, Debug)]
#[class(base = RefCounted, init)]
struct OptionFfiTest;

#[godot_api]
impl OptionFfiTest {
#[func]
fn return_option_refcounted_none(&self) -> Option<Gd<RefCounted>> {
None
}

#[func]
fn accept_option_refcounted_none(&self, value: Option<Gd<RefCounted>>) -> bool {
value.is_none()
}

#[func]
fn return_option_refcounted_some(&self) -> Option<Gd<RefCounted>> {
Some(RefCounted::new())
}

#[func]
fn accept_option_refcounted_some(&self, value: Option<Gd<RefCounted>>) -> bool {
value.is_some()
}

#[func]
fn mirror_option_refcounted(&self, value: Option<Gd<RefCounted>>) -> Option<Gd<RefCounted>> {
value
}

#[func]
fn return_option_node_none(&self) -> Option<Gd<Node>> {
None
}

#[func]
fn accept_option_node_none(&self, value: Option<Gd<Node>>) -> bool {
value.is_none()
}

#[func]
fn return_option_node_some(&self) -> Option<Gd<Node>> {
Some(Node::new_alloc())
}

#[func]
fn accept_option_node_some(&self, value: Option<Gd<Node>>) -> bool {
value.is_some()
}

#[func]
fn mirror_option_node(&self, value: Option<Gd<Node>>) -> Option<Gd<Node>> {
value
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like it should be possible to add these tests to the build.rs so they get auto-generated, can you do that if it is? dont worry about it if it would require rewriting the macro or something tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test generation does currently not play very well with Option. We need different tests for both Some(...) and None but the tests derive their name from the rust type. That makes it impossible to have tests for the same type with different values.
The accept methods are also implemented slightly different from what the code generation does because they operate on and check Options. I thought it's easier to just implement the tests manually, than to expand the complexity of the macros.