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

Implement more builtin types #10

Merged
merged 2 commits into from
Oct 16, 2022

Conversation

Waridley
Copy link
Collaborator

Copied work from the gdext-rust repo as discussed. Still has some TODO's but it should be usable and ready for at least an initial review.

Also prepares for the TypedArray support added by godotengine/godot#65817

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Hey, thanks a lot for this great PR! It looks mostly good, I had a few questions regarding my understanding.

It's especially cool that you extended the generator for more constructor functions, and in a generic way! Regarding Godot enums/bitfields, I agree the GDNative approach worked relatively well, but we can also evaluate some options (outside this PR).

gdext-builtin/src/macros.rs Outdated Show resolved Hide resolved
gdext-builtin/src/arrays.rs Show resolved Hide resolved
Comment on lines 51 to 48
pub fn get_mut(&mut self, index: i64) -> Option<&mut Variant> {
unsafe {
let ptr = (interface_fn!(array_operator_index))(self.sys(), index) as *mut Variant;
if ptr.is_null() {
return None;
}
Some(&mut *ptr)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

It's probably possible to get 2 Array instances pointing to the same GDScript array, right (e.g. through exported methods later)?

In that case, if you would do

let first: Array = ...;
let second: Array = ...; // same instance

let first_mut = first.get_mut(0);
let second_mut = second.get_mut(0); // aliased &mut -> UB

that creates a soundness hole, no?

If yes, it's fine for me to fix it later, but we might then add it to #5.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that would be a problem... I'm not yet clear on how safety is going to be different from what was done in GDNative. I know sometimes it will be GDScript's responsibility to not break the threading rules, but your example makes it possible to mutably alias just from Rust, which of course should probably be prevented. But then should even immutable access be unsafe just in case a mutable reference gets created on another thread in a similar way?

Copy link
Member

Choose a reason for hiding this comment

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

In GDNative, the methods are unsafe, so we "solved" it by telling the user "your problem now" 😁

I see multiple options to address this in the long run:

  1. We only give access to values, not references. We could still have utility methods like:
    pub fn replace<F>(&mut self, index: i64, replacer: F)
        where F: FnOnce(Variant) -> Variant
  2. We provide reference access, but unsafe. I'm not a big fan of this, as it will just make people write mindless unsafe blocks again, out of performance fear.
  3. We coordinate multiple Array instances among each other, and track references, RefCell style. This adds a lot of complexity and overhead though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Option 1 seems similar in philosophy to the Entry API in std. Hypothetically we could write our own Entry API for arrays and Dictionaries, but of course that would be more development work. But for every utility function we add, someone else will probably want another one we didn't add 😆 So I would probably want to still expose the unsafe accessors in case someone wants to do something specific, but discourage their use and suggest the utility functions instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, for option 1, threads could technically still be problematic.

We may need something like the Shared/Unique types somewhere...

Copy link
Member

Choose a reason for hiding this comment

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

Option 1 seems similar in philosophy to the Entry API in std. Hypothetically we could write our own Entry API for arrays and Dictionaries, but of course that would be more development work.

Interesting thought. I'm not opposed to that, but maybe we can plan it for a bit later?

What would we need to have the bare minimum functionality? get() and set() based on values? I'd probably rather do that now than start with unsafe get_ref() methods that we then need to remove 🤔 they're anyway limited in utility, because:

  • the one returning &Variant is semantically equivalent to returning Value, so it's mostly an (unsafe) optimization
  • the one returning &mut is also not much different from set, because Variant itself is immutable -- so all you could do is assign it directly or via mem::replace(), mem::swap() etc.

Also, for option 1, threads could technically still be problematic.

Unfortunately threads break every single safety assumption we're making, so we'll need the user to opt-in to either not using them or taking responsibility of thread safety themselves. The Unique/Shared model is also just a form of passing the responsibility to the user. It may help in some scenarios, but I'd like to keep the type system simple for now and see how far we get...

gdext-builtin/src/macros.rs Outdated Show resolved Hide resolved
gdext-codegen/src/central_generator.rs Outdated Show resolved Hide resolved
gdext-codegen/src/central_generator.rs Outdated Show resolved Hide resolved
@Bromeon Bromeon added feature Adds functionality to the library c: core Core components labels Oct 12, 2022
@Waridley Waridley force-pushed the feature/builtin_types branch 2 times, most recently from 6f7292f to b63a03c Compare October 13, 2022 04:12
@Bromeon
Copy link
Member

Bromeon commented Oct 13, 2022

Thanks for the updates!

Now the whole gdext-builtin/src/macros.rs has changed 😀 is it line endings?
What is your Git configuration of the core.autocrlf flag? I have set it to true on Windows, that should be good, no?

Another option would be to set up rustfmt with newline_style setting, but I prefer if people can use their own line-ending style locally, as long as Git commits it properly. Maybe only for CI or so.

@Waridley Waridley force-pushed the feature/builtin_types branch from b63a03c to a36e41a Compare October 15, 2022 22:05
@Waridley
Copy link
Collaborator Author

Thanks for the updates!

Now the whole gdext-builtin/src/macros.rs has changed grinning is it line endings? What is your Git configuration of the core.autocrlf flag? I have set it to true on Windows, that should be good, no?

Another option would be to set up rustfmt with newline_style setting, but I prefer if people can use their own line-ending style locally, as long as Git commits it properly. Maybe only for CI or so.

Mine is set to "input" on linux... Still not sure what happened, but it looks like one of your recent commits fixed it anyway. 🤷‍♂️

Are we planning to automatically drop types that have destructors, or should they only be manually freed? I implemented Drop for StringName in a separate commit to shush a Clippy lint about calling std::mem::forget on it, but I'm not sure if that's the right thing to do or not.

@Bromeon Bromeon merged commit 152b9ff into godot-rust:master Oct 16, 2022
@Bromeon Bromeon deleted the feature/builtin_types branch October 16, 2022 08:21
@Bromeon
Copy link
Member

Bromeon commented Oct 16, 2022

Thanks a lot for this! 🚀

Hapenia-Lans pushed a commit to Hapenia-Lans/gdextension that referenced this pull request May 26, 2023
# This is the 1st commit message:

Parse gdextension_interface.h declarations using regex

# This is the commit message #2:

AsUninit trait to convert FFI pointers to their uninitialized versions

# This is the commit message godot-rust#3:

GodotFfi::from_sys_init() now uses uninitialized pointer types

# This is the commit message godot-rust#4:

Introduce GDExtensionUninitialized*Ptr, without changing semantics

# This is the commit message godot-rust#5:

Adjust init code to new get_proc_address mechanism

# This is the commit message godot-rust#6:

Make `trace` feature available in godot-ffi, fix interface access before initialization

# This is the commit message godot-rust#7:

Compatibility layer between Godot 4.0 and 4.1 (different GDExtension APIs)

# This is the commit message godot-rust#8:

Add GdextBuild to access build/runtime metadata

# This is the commit message godot-rust#9:

Detect 4.0 <-> 4.1 mismatches in both directions + missing `compatibility_minimum = 4.1`

# This is the commit message godot-rust#10:

Detect legacy/modern version of C header (also without `custom-godot` feature)

# This is the commit message godot-rust#11:

CI: add jobs that use patched 4.0.x versions

# This is the commit message godot-rust#12:

Remove several memory leaks by constructing into uninitialized pointers

# This is the commit message godot-rust#13:

CI: memcheck jobs for both 4.0.3 and nightly

# This is the commit message godot-rust#14:

Remove ToVariant, FromVariant, and VariantMetadata impls for pointers

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.

# This is the commit message godot-rust#15:

Adds FromVariant and ToVariant proc macros

# This is the commit message godot-rust#16:

godot-core: builtin: reimplement Plane functions/methods

# This is the commit message godot-rust#17:

impl GodotFfi for Option<T> when T is pointer sized and nullable godot-rust#240

Additionally FromVariant and ToVariant are also implemented for Option<Gd<T>>
to satisfy all the requirements for ffi and godot_api.

# This is the commit message godot-rust#18:

Fix UB in virtual method calls that take objects
Fix incorrect incrementing of refcount when calling in to godot
Fix refcount not being incremented when we receive a refcounted object in virtual methods

# This is the commit message godot-rust#19:

fix UB caused by preload weirdness

# This is the commit message godot-rust#20:

Implements swizzle and converts from/to tuples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants