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

Separate out most of the low-level logic from Gd into RawGd #349

Closed
wants to merge 3 commits into from

Conversation

lilizoey
Copy link
Member

@lilizoey lilizoey commented Jul 18, 2023

Still a WIP. But the overall ideas should be close to what this will look like eventually.

There are a lot of things this PR does, so i've made a list of everything and what we can consider splitting out and such. When we've sorted all the things in the current todo we can figure out if we want to keep the RawGd object.

Legend:

emoji description
↪️ Should be split into another PR
↔️ Could be split into another PR
Should be kept in this PR
Unclear if we should keep this

Title format: emoji short title | full title

↪️ Object Unit Deref | Make Object not deref to ()

Since we set the base of Object to (), we also make it deref to (). This doesn't seem right. So we remove that deref.

↪️ VariantIsNull | Add VariantIsNull to VariantConversionError

Gd expects a variant to not be null, so this is a special case error for that.

Isn't a big change, so wouldn't make a big difference if it's split or not.

↔️ Gd Module | Move Gd and Base into a new module for smart pointers.

Gd contains some shared stuff between Gd and Base that we can move into a mod.rs, such as debug_string.

With this PR it'd be very useful to do this refactor to organize it well, but it could be started in another PR.

↪️ OpaqueObject To Pointer | Replace the OpaqueObject stored in Gd with an object pointer

Transmuting pointers to integers can be problematic.

⏬ RawGd | Add RawGd

This is the main thing this PR adds, it might be possible to make this more minimal than what this PR does but likely is gonna need to be kept as part of this PR.

⏬ RawGd Ffi | Move all direct ffi-calls from Gd to RawGd

RawGd is meant to provide a more rusty api over all the object related ffi-calls, and to be able to store any kind of object that is valid in godot. So we dont need to make the ffi-calls outside of RawGd or related methods.

↪️ Atomic InstanceId | Change the cached instance id from Cell<Option<InstanceId>> to AtomicU64

This is because RawGd should accomodate for all use-cases. However this might be worse in performance on some machines. We could probably make this a generic with default value for Gd somehow (and later RawGd if we make that split).

↪️ Deref Liveness | Make Deref panic if invalid instance, add unsafe as_inner(_mut)_unchecked for avoiding validity check

Fixes #348

↪️ Gd Safety Invariants | Document the safety invariants you need to uphold for Gd

We expect people to uphold these safety invariants from godot, but i dont think we explicitly document them. It's mainly about keeping the reference count live.

❓ ↪️ Singleton | Add Singleton smart-pointer

Add a smart pointer specifically for singletons. Since singletons have stronger liveness guarantees we can take advantage of.

It may be possible to do this with Gd. But it may still be useful even if technically doable with Gd. Further investigation needed.

❓ ↪️ Singleton Trait | Add unsafe GodotSingleton trait for classes that are singletons

Add an unsafe trait declaring that a type upholds the safety guarantees of a godot singleton.

We may be able to do this entirely using GodotClass::Mem.

↔️ InstanceId Convert | Add functions to convert Option<InstanceId> to i64/u64

Some QOL

↪️ Unsafe scoped_mut | Make Domain::scoped_mut unsafe

This function can be called on freed instances, and it isn't always safe to do that.

↪️ Unsafe maybe_dec_ref | Make Memory::maybe_dec_ref unsafe

This function can be used to invalidate a reference counted Gd, the other Memory methods wont ever do that, worst case they'll cause a memory leak

↪️ as_storage Ref | Make as_storage() return a &InstanceStorage instead of &mut InstanceStorage

Since we can't in general guarantee that as_storage will always return an exclusive reference. We do still need to create an exclusive reference in one case, when we drop the InstanceStorage. But in that case we can assume we have an exclusive reference and just manually do the cast.

↪️ InstanceStorage Cell | Make InstanceStorage's single-threaded lifecycle and godot_ref_count into Cell

Since we want to be able to modify them through shared references. Similar might need to be done in the threaded version.

↔️ GodotNullableFuncMarshal | Replace GodotNullablePtr with GodotNullableFuncMarshal

Doesn't rely as much on implementation details as GodotNullablePtr, and the GodotNullablePtr version doesn't work as well with RawGd, unless we reimplement GodotFfi for Gd. But that kinda goes against the point of RawGd.

↔️ drop_via | Add drop_via method to the GodotFuncMarshal

If a method needs to run some drop glue when the via returned from into_via is dropped. In this case Gd needs to decrement the refcount to avoid a memory leak.

↔️ ViaGuard | Add ViaGuard

To make running drop_via automatically easier.

Todo

Separate PRs for smaller things:

Figure out whether to split out or not

  • Gd Module
  • InstanceId Convert

Figure out if we should keep

  • Singleton
  • Singleton Trait
  • GodotNullableFuncMarshal
  • drop_via
  • ViaGuard

After all the above

  • Figure out if we should separate out RawGd

@GodotRust
Copy link

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

@Bromeon
Copy link
Member

Bromeon commented Jul 21, 2023

Thanks a lot for this great split!


⏬ RawGd Ffi | Move all direct ffi-calls from Gd to RawGd

RawGd is meant to provide a more rusty api over all the object related ffi-calls, and to be able to store any kind of object that is valid in godot. So we dont need to make the ffi-calls outside of RawGd or related methods.

Isn't that a bit of a sign that it's "too much abstracted" if it now is no longer specific enough to use the most-appropriate type for each use case? This could maybe be traited/genericked away, and we'd need to see if the differentiation after that is still simpler than having different implementations in the "frontend".

Regarding atomics in particular, I think a fair premise is that unless threads feature is enabled, we assume no multithreading that interferes with Rust code. We could add debug checks to protect against accidental use, but that's maybe a separate discussion.

Either way, I agree to split this off ↪️


Priority-wise, smaller things that can be done independently of Gd (or are required by potential refactorings) should probably be addressed first 🙂

@lilizoey
Copy link
Member Author

lilizoey commented Aug 6, 2023

with a separate RawGd, we might be able to make a ptrcall and varargcall method on RawGd. and have each engine class's method look something like this:

struct Object {
  raw: RawGd<Object>
}
// ...
impl Object {
  fn get(&self, property: StringName) -> Variant {
    let method_name = StringName::from("get");
    self.raw.ptrcall::<(Variant, StringName)>(method_name, (property,))
  }
}

@Bromeon
Copy link
Member

Bromeon commented Oct 4, 2023

Are there still many changes part of this PR, that are not in #421?

@lilizoey
Copy link
Member Author

lilizoey commented Oct 5, 2023

Are there still many changes part of this PR, that are not in #421?

I think most of the relevant stuff has been addressed in #421 or other places, so im just gonna close this now. Only thing i think we haven't really addressed much is special casing behavior for singletons more somehow. But i think we can take that conversation later.

@lilizoey lilizoey closed this Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

free can be used to cause a segfault entirely from rust
3 participants