Add get_property_list, property_can_revert and property_get_revert virtual methods

[lilizoey]

get_property_list in godot requires us to pass a pointer + length of GDExtensionPropertyInfo, so i needed to add some mechanism to store such an array of PropertyInfo until it gets freed by free_property_list. I added this in the Storage trait.

I also added new_var and new_export methods to PropertyInfo so people can more easily construct the property info when they need it, rather than needing to fill in all the fields manually.

I need to add some more tests and test it manually a bit more before i mark as ready for review.

A couple of alternatives to the storage method that i decided against would be:

Have the user be responsible for keeping this list of property infos alive

This would require some unsafe code and be quite unergonomic

Use some pointer shenanigans into a custom DST struct

Make a struct like

struct PropertyList {
    property_info: Vec<PropertyInfo>,
    sys_info: [sys::GDExtensionPropertyInfo],
}

then pass a pointer to sys_info to godot, then reconstruct this struct from the pointer godot gives us back by using pointer arithmetic to find the property_info field.

This is probably doable, but it requires a lot of unsafe finageling and seems unneccesarily complicated.

Directly free the vec from the pointer godot gives us in the free method

We have no good way of getting the length and capacity from just the pointer, so this isn't really feasible.

[GodotRust]

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

[Bromeon]

Nice, thanks!

It may be possible that multiple threads call get_property_list() on the same object simultaneously. This should generally be allowed, as it's semantically a read operation (but of course user code could use interior mutability). Multiple threads requesting the property list could result in a situation with overlapping get/free calls:

  Thread A       Storage      Thread B
                   |
      +<--- get ---+
      |            |
      |            |
      |            +--- get --->+
      |            |            |
      |            |            |
      v            v            |
      +--- free -->|            |
                   |            |
                   |            |
                   |<-- free ---+
                   v

This has two implications for experimental-threads:

1. The storage cannot rely on only a single property being "in use", i.e. it shouldn't perform destructive operations in free.

   • What would work is a stack-based design with atomic ints (+1 on get, -1 on free, destroy if 0).

2. We need to see how we make the user function thread-safe. Multiple options:

   1. Cache the results of get_property_list() on first invocation, only call user function once.
      This may not be very true to the Godot paradigm; it might cause issues if someone wants to implement a dynamic system.
   2. Run get_property_list() from multiple threads -- this would mean it can't have access to &mut self but needs the (not yet implemented) multithreaded equivalent of Gd<T>, or at least some lock.
   3. Some sort of synchronization, to at least make sure access is not concurrent. It will be hard to reroute all access through main thread though...
   4. Panic if not invoked on main thread. This is anyway the go-to for single-threaded mode, and it's perfectly fine for me to make this more restrictive for multi-threading now and handle it properly later (when we possibly have more insight in our object threading design).

There's an unlikely chance that Godot already takes some measures to ensure this is always called from the main thread, which would of course make this easier. We should probably test this 😉

[lilizoey]

that's true, i'll look into that a bit more.

   1. Cache the results of `get_property_list()` on first invocation, only call user function once.
      This may not be very true to the Godot paradigm; it might cause issues if someone wants to implement a dynamic system.

This is one of the main use-cases of get_property_list though, so it'd suck to break that.

   2. Run `get_property_list()` from multiple threads -- this would mean it can't have access to `&mut self` but needs the (not yet implemented) multithreaded equivalent of `Gd<T>`, or at least some lock.

I mean the &mut self thing isn't really an issue as the code all uses immutable references here. the bigger issue is that we invalidate the reference to the property list.

   3. Some sort of synchronization, to at least make sure access is not concurrent. It will be hard to reroute all access through main thread though...
   4. Panic if not invoked on main thread. This is anyway the go-to for single-threaded mode, and it's perfectly fine for me to make this more restrictive for multi-threading now and handle it properly later (when we possibly have more insight in our object threading design).

There's an unlikely chance that Godot already takes some measures to ensure this is always called from the main thread, which would of course make this easier. We should probably test this 😉

Part of the issue here is that we need to keep PropertyInfo around while the sys::GDExtensionPropertyInfo exists. But could it be possible to just leak the data from PropertyInfo, then later we just reconstruct the PropertyInfo and free it properly then? if so then we could maybe avoid storing any intermediate state.

Like the only reason PropertyInfo needs to be kept around is because when it's dropped the class_name, property_name and hint_string might get freed. but if those are just forgotten (and we then pass ownership to godot) then that shouldn't be an issue?

[Bromeon]

that's true, i'll look into that a bit more.

I noticed that multi-threaded access may also be an issue for other virtual functions like set_property, to_string etc. It's a just a bit clearer here due to the temporally spaced-out get/free calls, but race conditions could theoretically also occur in the other places.

So it's also fine if we handle the single-threaded case well here, and think about the multithreaded one in a separate issue/PR.

---

Part of the issue here is that we need to keep PropertyInfo around while the sys::GDExtensionPropertyInfo exists. But could it be possible to just leak the data from PropertyInfo, then later we just reconstruct the PropertyInfo and free it properly then? if so then we could maybe avoid storing any intermediate state.

We do that in other places, e.g. script_instance.rs -- but there's usually a user-data void* around, or we can simply convert pointers.

Here, we'd probably need to construct the struct "around" the pointer -- which is possible, but would need to assume the addresses before/after it are occupied in a certain way?

[lilizoey]

Part of the issue here is that we need to keep PropertyInfo around while the sys::GDExtensionPropertyInfo exists. But could it be possible to just leak the data from PropertyInfo, then later we just reconstruct the PropertyInfo and free it properly then? if so then we could maybe avoid storing any intermediate state.

We do that in other places, e.g. script_instance.rs -- but there's usually a user-data void* around, or we can simply convert pointers.

Here, we'd probably need to construct the struct "around" the pointer -- which is possible, but would need to assume the addresses before/after it are occupied in a certain way?

I think i found something that should work. I'll add two methods to PropertyInfo, into_property_sys which consumes the property info and leaks the property name + hint string (we dont need to do anything with class name afaik since it's never dropped in the first place). and an unsafe function drop_property_sys which takes a sys::GDExtensionPropertyInfo and drops the property name + hint string.

Then in get_property_list, i create a "null"-terminated array of the property infos (null here meaning an invalid property info that cannot be constructed from PropertyInfo), and pass it to godot.

Finally in free_property_list, i traverse the array pointer godot returns to find the null so i know the length, reconstruct the vec, and call drop_property_sys on each of them (except the final invalid one).

[Bromeon]

Probably interesting: godotengine/godot#89055 👀

To me it's also fine to only support this in Godot 4.3+, then we don't need to maintain 2 implementations.

[dsnopek]

@Bromeon I think this actually pertains to the free_property_list_func on GDExtensionClassCreationInfo2, which isn't addressed by PR godotengine/godot#89055 (instead, that PR is concerned with GDExtensionScriptInstanceInfo2).

We probably need another PR for Godot, which would be convenient to do now, because we already have a GDExtensionClassCreationInfo3 (2 to 3) for other changes in Godot 4.3.

EDIT: Or maybe I'm wrong and this is about script instances? I'm not great at interpreting the Rust code here. :-) I guess let me know and we can figure out if there's another struct that needs updating

[lilizoey]

So if the free property list function gets updated in 4.3 to pass the length of the property list, should we then:

1. Use one solution that works both before and after 4.3
2. Use different solutions for <4.3 and 4.3+
3. Only support property lists in 4.3+?

It would be a bit unfortunate to need both the <4.3 version as well as the 4.3+ version, but the 4.3+ version will likely be a lot simpler so it'd also be unfortunate to only use the more complicated solution. We dont currently support property lists for <4.3 anyway so maybe it's fine to just not support that?

[lilizoey]

i have an idea actually,

we add a const value PROPERTY_LIST_CAPACITY to the I* traits

const PROPERTY_LIST_CAPACITY: usize = 16;
// or some other reasonable default

in godot <4.3 we always ensure that the property has a capacity (and length?) of the value that returns is exactly 16, and so we can always just assume that that's the size.

in 4.3+ we can depracate this and just use the length returned by godot

thus most of the code can be the same for the two versions, they largely just differ in where they get the length from.

[Bromeon]

We dont currently support property lists for <4.3 anyway so maybe it's fine to just not support that?

Yep, that's what I meant with this: 🙂

To me it's also fine to only support this in Godot 4.3+, then we don't need to maintain 2 implementations.

No one has asked for this feature, so it's well possible that any extra effort to support it for Godot 4.2 is wasted. We already invest a lot of extra time to maintain compatibility with previous Godot minor versions (more than the other bindings), not sure if we should always backport new features as well.

So I wouldn't go any extra lengths here (not even PROPERTY_LIST_CAPACITY), starting in 4.3 is fine 👍

[Bromeon]

Any update on this?

[lilizoey]

I dont think there's been any upstream progress on adding a count to the property list for gdextension classes. So without that we can do something like what was done in #650 or wait until that happens. I may possibly be able to make a PR upstream for that myself but i am not very experienced with c++ so it'd be a bigger commitment.

[Bromeon]

Maybe before committing a lot of time, you could open an upstream issue first and see what others think about it? Doesn't have to be you to do the entire implementation 🙂

[lilizoey]

I guess this would be a proposal not a bug report, arguably you could say that it's a bug that script instance passes count to the free methods and gdextension does not. but i think it is a bit more of a stretch than saying it's a proposal/enhancement.

[lilizoey]

i made this issue godotengine/godot-proposals#9462

[lilizoey]

Closing this for now so it doesn't perpertually show up as a PR, opening the issue #665 instead.