Implement virtual methods that accept or return pointers (per #191) by Mercerenies · Pull Request #272 · godot-rust/gdext (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation54 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This MR adds all missing virtual functions which were previously omitted due to an argument or return value being a pointer. All pointers are translated into Rust-side raw pointers (*const T or *mut T, as appropriate). All virtual trait functions which either take or return a pointer are marked as unsafe.

All native_structures structs in the JSON file have been translated into Rust-side structures (with #[repr(C)] to ensure binary compatibility) and placed in the godot::native directory. These are all pure data structs (all fields are public and they have no methods). Many of the pointer functions take or return pointers to these native structures, so being able to construct and access them Rust-side is essential for this functionality.

There is one double pointer in the JSON API. const uint8_t** appears in several of the networking functions. I believe the correct translation of this type (preserving const) is *mut *const u8, which is what the code in this MR uses.

Bromeon added feature

Adds functionality to the library

c: ffi

Low-level components and interaction with GDExtension API

labels

May 11, 2023

Choose a reason for hiding this comment

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

Thanks a lot for this pull request! 😀

Added some comments in the code.

I'm not very sure if generally implementing ToVariant/FromVariant for pointer types is desired. Is there any use case outside of native structures? If not, we may have to bite the bullet and split the SignatureTuple trait, as discussed on Discord 🤔 more thoughts on this?

If you rebase this code on latest master, the godot-itest failure should go away and the license-guard one should become more expressive (every file needs a license header).

Bromeon linked an issue

May 11, 2023

that may beclosed by this pull request

I'm not very sure if generally implementing ToVariant/FromVariant for pointer types is desired. Is there any use case outside of native structures? If not, we may have to bite the bullet and split the SignatureTuple trait, as discussed on Discord thinking more thoughts on this?

Regarding this, I don't think there's a use case in "ordinary" Godot code (read: Anything that doesn't end in the word "Extension"). But I personally think it's a lot nicer tunneling through a Variant than splitting (I wasn't a fan of the splitting idea, which is why I was hoping you or lili had an alternative suggestion). And there's precedent: we already have nontrivial conversions in ToVariant and FromVariant for things like smaller fixed-size integer types and the infamous Option<Gd<T>>. So I think it's reasonable to treat FromVariant / ToVariant as a general "serialization-to-Godot" framework, and under that interpretation casting a pointer to an integer seems like a perfectly reasonable serialization technique in my mind.

The question is: should this be part of the public API?

So I think it's reasonable to treat FromVariant / ToVariant as a general "serialization-to-Godot" framework, and under that interpretation casting a pointer to an integer seems like a perfectly reasonable serialization technique in my mind.

I'm actually more worried about the other side: "deserialization-from-Godot", aka. FromVariant, aka. construct pointers out of thin air. So far, the Variant conversions are something very high-level, relatively easy to reason about, with no real error potential (Result catches those).

If we allow Variant(Int) to be converted to raw pointers, this can very easily lead to invalid and unchecked results.

let variant = 32.to_variant(); let ptr = variant.to::<*mut String>();

While it is technically safe, we essentially committed a mem::transmute() through variant conversions. You do need extra unsafe to dereference the pointers, but still -- this seems very easy to use wrong and brings FFI complexity into the high-level Variant API.

And there's precedent: we already have nontrivial conversions in ToVariant and FromVariant for things like smaller fixed-size integer types and the infamous Option<Gd<T>>.

But they always return a valid result, or the conversion fails as Result::Err. AFAIK it's not possible to construct invalid values (otherwise it might be a bug).

Regarding this, I don't think there's a use case in "ordinary" Godot code (read: Anything that doesn't end in the word "Extension").

Maybe that's an indicator that it deserves a more specialized API than the high-level Variant one (or go through the split traits directly). We also need to consider that adding raw pointer conversions increases the complexity of the Variant API, while 99% of users won't ever benefit from those corner cases.

Either way, if you modify it, please preserve the current commit (Variant conversions to/from pointers) and add more on top; this might be something interesting to potentially revisit at some point. Even if it's just for the design rationale.

Choose a reason for hiding this comment

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

Your last merge seems to have gone wrong; you are now reverting changes from the master branch.

I just mentioned a few files, but many more are affected.

Choose a reason for hiding this comment

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

Your seem to accidentally revert several unrelated changes from master. Please undo those changes.

Choose a reason for hiding this comment

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

Also in this file.

Choose a reason for hiding this comment

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

And here.

@Bromeon Oof, sorry about the botched rebase. I think it should be repaired now. Let me know if anything still looks suspicious.

I see your argument regarding FromVariant producing raw pointers. I'm going to think about it a bit and take another look at it tomorrow. If I don't have any better ideas, I'll implement the split traits. If I do, I'll mention it.

Added commit to split the signature tuple trait. It ended up being far simpler than I thought (the macro calls just automagically know which trait they need; there wasn't a lot of manual correction to be done).

I'm actually more worried about the other side: "deserialization-from-Godot", aka. FromVariant, aka. construct pointers out of thin air. So far, the Variant conversions are something very high-level, relatively easy to reason about, with no real error potential (Result catches those).

If we allow Variant(Int) to be converted to raw pointers, this can very easily lead to invalid and unchecked results.

let variant = 32.to_variant(); let ptr = variant.to::<*mut String>();

While it is technically safe, we essentially committed a mem::transmute() through variant conversions. You do need extra unsafe to dereference the pointers, but still -- this seems very easy to use wrong and brings FFI complexity into the high-level Variant API.

This is equivalent to doing

let a: usize = 32; let ptr = a as *mut String;

Which is entirely legal in safe rust. i dont think this is an argument against allowing this conversion. Unless you're suggesting we change to using strict provenance, then this is entirely fine within rust. But using strict provenance would be a big change.

But they always return a valid result, or the conversion fails as Result::Err. AFAIK it's not possible to construct invalid values (otherwise it might be a bug).

You can't construct invalid values in the sense of unsafety or unsoundness. But we can return semantically invalid values. Like returning a Rect2 with negative size. A pointer we can't dereference is just a pointer we can't use. But you already know to guard against these issues when you're dealing with pointers in rust.

as is not very complicated in rust in general, and saying that FromVariant/ToVariant becomes overly complicated because we allow it to do conversion that as does seems very strange to me.

An alternative solution here would be to leverage the Via in GodotFuncmarshal. And only require in Signature that the Via of GodotFuncMarshal implements FromVariant/ToVariant. Then we implement GodotFuncMarshal for pointers using i64 as the Via. This would then not expose FromVariant/ToVariant conversions of pointers directly, but would still allow us to pass around pointers through functions.

This would require a fair bit of rewriting though, especially as it seems like we're misusing the Via a bit, using Infallible for the Via to indicate a conversion is infallible. In reality GodotFuncMarshal should probably look like:

pub trait GodotFuncMarshal: Sized { /// Intermediate type through which Self is converted. type Via; /// The type returned when conversion fails type Error: Debug; ... }

This is equivalent to doing

let a: usize = 32; let ptr = a as *mut String;

Which is entirely legal in safe rust

I'm aware of that, which is precisely why I mentioned "technically safe". You can write 1000i64 as i8, it's safe as well. Is it good practice though?

as casts are the crowbar in the Rust type system. Their power is equivalent to static_cast, const_cast and reinterpret_cast of C++ combined. Which says a lot, given that C++'s motivation of introducing those keywords was to split the power of C-style casts in the first place. When it comes to pointer casts, I found that as is most often better used inside abstractions that have very defined conversions (such as GodotFfi or sys::force_mut_ptr).

Memory safety is irrelevant here, what I'm talking about is the potential to introduce bugs. And I'm not a fan of APIs that make this easy, especially if they mostly exist to serve internal implementation.

An alternative solution here would be to leverage the Via in GodotFuncmarshal.

It looks like the latest revision of this PR works without ToVariant/FromVariant and is relatively simple, so that looks good to me.

This commit splits SignatureTuple into two separate traits: PtrcallSignatureTuple and VarcallSignatureTuple. The latter is a child of the former. PtrcallSignatureTuple is used for ptrcall and only demands GodotFuncMarshall of its arguments. VarcallSignatureTuple is used for varcall and additionally demands ToVariant, FromVariant, and VariantMetadata of its arguments, so pointers cannot benefit from the optimizations provided by varcall over ptrcall.

bors bot added a commit that referenced this pull request

May 18, 2023

@Bromeon Are those bors failures merge conflicts? They don't look like parts of the codebase I've touched. What is bors doing that ./check.sh isn't?

No, those point to an upstream change with Godot (the gdextension_interface.h changed, and we need to update first).

I'm already working on this, no action needed from your side 🙂

bors bot added a commit that referenced this pull request

May 24, 2023

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

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

Thanks a lot for this addition and the patience! 🚀

bors bot added a commit that referenced this pull request

Jun 17, 2023

314: ApproxEq trait + refactorings r=Bromeon a=Bromeon

Changes:

Add ApproxEq trait and implement it for many builtin types.

• This standardizes checking for approximate equality across types.
• Gets rid of the boilerplate 3rd argument assert_eq_approx!(a, b, is_equal_approx).
  • That argument can still be specified if desired, using assert_eq_approx!(a, b, fn = is_equal_approx).
  • In 95% of cases, it's not needed though.

Refactor modules

• godot::builtin::math is now a public module, which contains:
  • Godot ported functions such as lerp, sign
  • ApproxEq trait + assert_eq_approx!, assert_ne_approx! macros
  • glam utilities: IVec4, GlamConv, etc (internal)
• The above symbols are no longer in godot::builtin. If this turns out to be an issue, we can also revert this in the future; but it might help some discoverability. Some symbols could also be considered to be part of the prelude.
• Move godot::native_structure -> godot::engine::native
  • Added in #272
  • Native structures are now less prominent because rarely used.
• Move real-related symbols into their own real.rs file (internal; API unaffected).

Refactor import statements (just around geometric types, not whole codebase)

• Remove wildcard imports
• Remove nested import statements
• Consistent order

Co-authored-by: Jan Haller bromeon@gmail.com

Labels

c: engine

Godot classes (nodes, resources, ...)

c: ffi

Low-level components and interaction with GDExtension API

feature

Adds functionality to the library