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

TitanNano · 2023-04-27T22:46:55Z

ToVariant and FromVariant are implemented for Option<Gd<T>> as well as they are a requirement of #[godot_api].

Closes #240.

godot-ffi/src/godot_ffi.rs

lilizoey · 2023-04-27T22:54:32Z

if you wanna talk a bit about design for this, feel free to check out the discord thread for this issue. i've been experimenting with a couple of things there.

if this just works then that'd be great, but i didn't really think this would work as an implementation.

lilizoey

i dont think doing

let nil_ptr = ptr::null_mut();
interface_fn!(variant_new_nil)(nil_ptr);

is correct, since you're telling godot to initialize a new object into a null-pointer. im a bit surprised it works.

also is there a reason you can't just pass around null-pointers directly in this code? does that break something?

also could you change the title to reflect what the PR actually does.

godot-ffi/src/godot_ffi.rs

TitanNano · 2023-04-29T11:31:07Z

if you wanna talk a bit about design for this, feel free to check out the discord thread for this issue. i've been experimenting with a couple of things there.

if this just works then that'd be great, but i didn't really think this would work as an implementation.

Unfortunately, I don't use Discord, so this thread is completely in accessible to me.

The initial changes did indeed not really work for Option::None but I have revised it, and so far, I am able to pass null and Node3D as arguments from GDScript to Rust. Returning Option::None and Some() also work but as I explained in another comment, it doesn't really make sense for primitive types.

lilizoey · 2023-04-29T11:54:22Z

if you wanna talk a bit about design for this, feel free to check out the discord thread for this issue. i've been experimenting with a couple of things there.
if this just works then that'd be great, but i didn't really think this would work as an implementation.

Unfortunately, I don't use Discord, so this thread is completely in accessible to me.

The initial changes did indeed not really work for Option::None but I have revised it, and so far, I am able to pass null and Node3D as arguments from GDScript to Rust. Returning Option::None and Some() also work but as I explained in another comment, it doesn't really make sense for primitive types.

right, that is what i initially assumed. I'll have a closer look at everything later but to summarize a bit from the discord thread:

the basic issue is rust orphan rules. as you might have noticed, it's not possible to blanket implement

unsafe impl<T: GodotClass> GodotFfi for Option<Gd<T>> {}

so how do we work around this?

your idea does work, but it means we now have a bunch of not really useful types. but if it doesn't break anything then it's not the worst honestly. though I'd need more time to be sure that there's no UB.

if we want to only implement it for Option<Gd<T>> then i essentially had three ideas.

first, use some kind of newtype representing a nullable Gd. im not sure how possible this is, as i never tested it, and it might require users to use that nullable Gd type. which isn't ideal.

second, create a new trait something like NullableGodotFfi which is just like GodotFfi but takes/returns Option<Self>. then blanket impl

impl<T: NullableGodotFfi> GodotFfi for Option<T>

now we implement both GodotFfi and NullableGodotFfi for Gd<T>.
this works perfectly but requires duplicating basically the entire trait which isnt ideal. i was trying to think of ways to avoid the duplication somehow but couldn't quite think of anything.

thirdly, we could move GodotFuncMarshal into godot-core. this would let us blanket implement it for Option<Gd<T>>. this will likely work, but im not entirely sure if it's the right thing to do. especially as we might like to split up core eventually for performance reasons.

TitanNano · 2023-04-30T10:24:26Z

second, create a new trait something like NullableGodotFfi which is just like GodotFfi but takes/returns Option. then blanket impl
impl<T: NullableGodotFfi> GodotFfi for Option<T>
[...]

this works perfectly but requires duplicating basically the entire trait which isnt ideal. i was trying to think of ways to avoid the duplication somehow but couldn't quite think of anything.

Why not just a simple marker trait?

trait NullableGodotFfi: GodotFfi {}

and we constraint the impl of GodotFfi for Option<T> to that, i.e.

impl<T: NullableGodotFfi> GodotFfi for Option<T> { ... }

lilizoey · 2023-04-30T12:08:07Z

second, create a new trait something like NullableGodotFfi which is just like GodotFfi but takes/returns Option. then blanket impl
impl<T: NullableGodotFfi> GodotFfi for Option<T>
[...]
this works perfectly but requires duplicating basically the entire trait which isnt ideal. i was trying to think of ways to avoid the duplication somehow but couldn't quite think of anything.
Why not just a simple marker trait?
trait NullableGodotFfi: GodotFfi {}
and we constraint the impl of GodotFfi for Option<T> to that, i.e.
impl<T: NullableGodotFfi> GodotFfi for Option<T> { ... }

that probably works yeah! if you make it unsafe and add the safety requirement that anything that implements the trait is nullable and pointer-sized then we should be good to use your implementation (maybe with a couple of tweaks still). since we can then rely on the guarantee that anything that implements that trait is pointer sized.

maybe a different name would be more appropriate too. since it declares that the type is both nullable and pointer-sized in godot. though from what you're saying it sounds like every type that is nullable is pointer-sized. but i feel like it'd still be good for the name to reflect that.

if it turns out there are more nullable types, or godot adds other nullable types, then we can tweak the trait slightly.

lilizoey · 2023-04-30T12:28:17Z

a couple of things. could you change the name of this PR to reflect what the PR actually does, rather than the issue it fixes? (such as "implements GodotFfi for Option<Gd<T>>"). and could you add "closes #240" to the description of the PR?

TitanNano · 2023-05-01T20:36:24Z

though from what you're saying it sounds like every type that is nullable is pointer-sized. but i feel like it'd still be good for the name to reflect that.

if it turns out there are more nullable types, or godot adds other nullable types, then we can tweak the trait slightly.

For completeness’s sake and to document my findings:

the current Godot 4.0 source indicates that only Object and types that inherit from it are nullable based on these methods:

https://github.com/godotengine/godot/blob/9f12e7b52d944281a39b7d3a33de6700c76cc23a/modules/gdscript/gdscript_analyzer.cpp#L4966

https://github.com/godotengine/godot/blob/9f12e7b52d944281a39b7d3a33de6700c76cc23a/core/variant/variant.cpp#L511

lilizoey

This looks good! Only a couple of minor things.

I'm not sure about the name GodotNullablePtrSized but i also am not entirely sure what would be a better name than that.

I'm very interested in seeing if the memory checked builds in the CI will be happy with this.

bors try

godot-ffi/src/godot_ffi.rs

lilizoey · 2023-05-03T21:17:43Z

itest/rust/src/option_ffi_test.rs

+    fn mirror_option_node(&self, value: Option<Gd<Node>>) -> Option<Gd<Node>> {
+        value
+    }
+}


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.

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.

godot-core/src/obj/gd.rs

bors · 2023-05-03T21:39:07Z

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

full-ci

Bromeon

Thanks a lot for this addition! 🙂

@lilizoey already mentioned a lot, I agree with their comments.

Regarding GodotNullablePtrSized:

It's not obvious at first sight why GodotFfi wasn't implemented directly on Option<Gd<T>>. Could you document that it exists primarily because of the orphan rule, that is, because GodotFfi and Gd live in separate crates? The existing general doc about the nullable pointer counterpart in Godot is great too, but you could add that there's currently just one concrete implementor: Option<Gd<T>>.
Maybe name it simply GodotNullablePtr? The Sized makes people think of Rust's Sized marker.

godot-core/src/obj/gd.rs

godot-ffi/src/godot_ffi.rs

Bromeon

Thanks for the update! Could you also fix the merge conflicts? 🙂

godot-core/src/obj/gd.rs

godot-ffi/src/godot_ffi.rs

GodotRust · 2023-05-10T21:48:12Z

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

TitanNano · 2023-05-18T14:21:42Z

@lilizoey @Bromeon does one of you have time to check this out again?

lilizoey

Seems basically good to me at least!

godot-core/src/obj/gd.rs

Bromeon · 2023-05-18T17:11:14Z

At the moment we can't merge because there are upstream breaking changes in gdextension_interface.h.
I'm currently updating those.

Bromeon · 2023-05-24T18:49:55Z

bors try

Bromeon · 2023-05-24T19:30:46Z

bors p=8

bors · 2023-05-24T20:03:41Z

try

Build failed:

full-ci

…t-rust#240 Additionally FromVariant and ToVariant are also implemented for Option<Gd<T>> to satisfy all the requirements for ffi and godot_api.

Bromeon · 2023-05-24T21:25:50Z

Thanks for the quick update and the patience! 🚀

bors r+

bors · 2023-05-24T21:34:57Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

full-ci

TitanNano commented Apr 27, 2023

View reviewed changes

godot-ffi/src/godot_ffi.rs Outdated Show resolved Hide resolved

lilizoey reviewed Apr 27, 2023

View reviewed changes

godot-ffi/src/godot_ffi.rs Outdated Show resolved Hide resolved

godot-ffi/src/godot_ffi.rs Outdated Show resolved Hide resolved

godot-ffi/src/godot_ffi.rs Outdated Show resolved Hide resolved

TitanNano force-pushed the jovan/240 branch from 7a48845 to 143ca27 Compare April 28, 2023 22:59

TitanNano commented Apr 29, 2023

View reviewed changes

godot-ffi/src/godot_ffi.rs Outdated Show resolved Hide resolved

lilizoey added feature Adds functionality to the library c: engine Godot classes (nodes, resources, ...) labels Apr 29, 2023

TitanNano force-pushed the jovan/240 branch from 143ca27 to f2ed6fa Compare May 1, 2023 20:24

TitanNano changed the title ~~Option<Gd<T>> does not implement ToVariant #240~~ impl GodotFfi for Option<T> when T is pointer sized and nullable #240 May 1, 2023

TitanNano changed the title ~~impl GodotFfi for Option<T> when T is pointer sized and nullable #240~~ impl GodotFfi for Option<T> when T is pointer sized and nullable #240 May 1, 2023

TitanNano force-pushed the jovan/240 branch from f2ed6fa to b9d7fec Compare May 1, 2023 20:29

TitanNano requested a review from lilizoey May 3, 2023 19:51

lilizoey reviewed May 3, 2023

View reviewed changes

bors bot added a commit that referenced this pull request May 3, 2023

Try #247:

0a3819b

lilizoey reviewed May 3, 2023

View reviewed changes

godot-core/src/obj/gd.rs Outdated Show resolved Hide resolved

lilizoey requested a review from Bromeon May 3, 2023 21:40

lilizoey mentioned this pull request May 8, 2023

Implement ToGodot/FromGodot for Vec<T> #263

Closed

Bromeon reviewed May 8, 2023

View reviewed changes

godot-core/src/obj/gd.rs Outdated Show resolved Hide resolved

godot-ffi/src/godot_ffi.rs Show resolved Hide resolved

TitanNano force-pushed the jovan/240 branch from b9d7fec to 5e36f13 Compare May 9, 2023 23:33

TitanNano requested review from Bromeon and lilizoey May 9, 2023 23:39

Bromeon reviewed May 10, 2023

View reviewed changes

godot-core/src/obj/gd.rs Outdated Show resolved Hide resolved

godot-ffi/src/godot_ffi.rs Outdated Show resolved Hide resolved

godot-ffi/src/godot_ffi.rs Outdated Show resolved Hide resolved

TitanNano force-pushed the jovan/240 branch from 5e36f13 to cb4c97a Compare May 10, 2023 21:42

TitanNano requested a review from Bromeon May 11, 2023 17:14

lilizoey reviewed May 18, 2023

View reviewed changes

godot-core/src/obj/gd.rs Outdated Show resolved Hide resolved

TitanNano force-pushed the jovan/240 branch from cb4c97a to 5d4c622 Compare May 22, 2023 18:27

gg-yb mentioned this pull request May 23, 2023

Crash caused by Godot Editor allowing inserting null into Array<Gd<T>> #283

Open

bors bot added a commit that referenced this pull request May 24, 2023

Try #247:

444e6ff

impl GodotFfi for Option<T> when T is pointer sized and nullable godo…

1fb9642

…t-rust#240 Additionally FromVariant and ToVariant are also implemented for Option<Gd<T>> to satisfy all the requirements for ffi and godot_api.

TitanNano force-pushed the jovan/240 branch from 5d4c622 to 1fb9642 Compare May 24, 2023 21:06

bors bot merged commit 9e6a8a0 into godot-rust:master May 24, 2023

TitanNano deleted the jovan/240 branch May 24, 2023 21:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

TitanNano commented Apr 27, 2023 •

edited

Loading

lilizoey commented Apr 27, 2023

lilizoey left a comment •

edited

Loading

TitanNano commented Apr 29, 2023

lilizoey commented Apr 29, 2023 •

edited

Loading

TitanNano commented Apr 30, 2023

lilizoey commented Apr 30, 2023 •

edited

Loading

lilizoey commented Apr 30, 2023 •

edited

Loading

TitanNano commented May 1, 2023 •

edited

Loading

lilizoey left a comment

lilizoey May 3, 2023

TitanNano May 9, 2023

bors bot commented May 3, 2023

Bromeon left a comment •

edited

Loading

Bromeon left a comment

GodotRust commented May 10, 2023

TitanNano commented May 18, 2023

lilizoey left a comment

Bromeon commented May 18, 2023

Bromeon commented May 24, 2023

Bromeon commented May 24, 2023

bors bot commented May 24, 2023

Bromeon commented May 24, 2023

bors bot commented May 24, 2023

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

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

Conversation

TitanNano commented Apr 27, 2023 • edited Loading

lilizoey commented Apr 27, 2023

lilizoey left a comment • edited Loading

Choose a reason for hiding this comment

TitanNano commented Apr 29, 2023

lilizoey commented Apr 29, 2023 • edited Loading

TitanNano commented Apr 30, 2023

lilizoey commented Apr 30, 2023 • edited Loading

lilizoey commented Apr 30, 2023 • edited Loading

TitanNano commented May 1, 2023 • edited Loading

lilizoey left a comment

Choose a reason for hiding this comment

lilizoey May 3, 2023

Choose a reason for hiding this comment

TitanNano May 9, 2023

Choose a reason for hiding this comment

bors bot commented May 3, 2023

try

Bromeon left a comment • edited Loading

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

GodotRust commented May 10, 2023

TitanNano commented May 18, 2023

lilizoey left a comment

Choose a reason for hiding this comment

Bromeon commented May 18, 2023

Bromeon commented May 24, 2023

Bromeon commented May 24, 2023

bors bot commented May 24, 2023

try

Bromeon commented May 24, 2023

bors bot commented May 24, 2023

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

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

TitanNano commented Apr 27, 2023 •

edited

Loading

lilizoey left a comment •

edited

Loading

lilizoey commented Apr 29, 2023 •

edited

Loading

lilizoey commented Apr 30, 2023 •

edited

Loading

lilizoey commented Apr 30, 2023 •

edited

Loading

TitanNano commented May 1, 2023 •

edited

Loading

Bromeon left a comment •

edited

Loading