Closed Mercerenies closed 5 months ago
I believe the script instance should either adopt GodotCell
once it is ready, or an alternative I already proposed in the initial PR, is to make the entire script instance &self
and delegate all mutation to the script instance implementation as interior mutability.
That said, script recursion should not require going back through the engine and should be contained in the script runtime.
EDIT: I missed that GodotCell
has already been merged.
That said, script recursion should not require going back through the engine and should be contained in the script runtime.
I'm not sure I agree with that. If I have an unknown object of type Variant
, I'm going to call back into the Godot engine to invoke methods on it, not handle that myself. Sure, if it's explicit, direct recursion then I could do something clever and never leave Rust, but that doesn't handle mutual recursion, recursion that goes through GDScript, or calling something like Array.map
or another higher-order Godot function with a function argument that calls back into self
somehow. There are lots of legitimate reasons to recurse through Godot.
@Mercerenies All right, I was more thinking of direct recursion. But what you are describing is definitely an issue. Unfortunately, it is a general issue of gdext.
If I'm not mistaken, even with GodotCell
call stack like
Class::method(&mut self) -> EngineClass::api_method() -> Class::method(&self)
will cause a panic.
Unfortunately, it is a general issue of gdext.
If I'm not mistaken, even with
GodotCell
call stack likeClass::method(&mut self) -> EngineClass::api_method() -> Class::method(&self)
will cause a panic.
No, GodotCell
should solve this π
You can look at an integration test to get a feel. Here you have the chain first_calls -> call -> second
.
@Bromeon sorry I didn't make my example super clear.
So yes, the new GodotCell
implementation allows for re-entrance when making calls to the API if the base class. What I was trying to express though is any other scenario where a call to some engine API happens to call back into the same rust class again.
Something like this:
CustomScriptLanguage::method(&mut self) -> Engine::api_method() -> CustomScriptLanguage::get_type(&self)
Or a class calling a GDScript that calls back into the class due to some circular references:
impl CustomNode {
pub fn method(&mut self) {
self.ref_gdscript_child_node.call("some_method", &[]);
}
pub fn on_child_signal(&self) {...}
}
func some_method():
self.child_signal.emit()
This discussion leads me to this idea of performing a "downgrade" of &mut self
to &self
before calling an engine API:
let base = self.base_mut();
let self_gd = base.to_godot().cast::<Self>();
let new_self = self_gd.bind();
@Mercerenies what are your thoughts on making the entire ScriptInstance
take &self
? I still believe it would give implementors the highest amount of control and flexibility.
Can we do that? To be honest, I would've written that idea off as impossible, but I suppose if everything really is just clever interior mutability, then we could probably get away with it. If &self
ing everything is remotely possible, then I'm on board with it. And (again, assuming there's no unexpected hiccups) it would certainly be the most straightforward thing to implement.
I also feel like we have too much direct access to ScriptInstance
. Every other Godot type is a Gd<T>
, so when we pass them around and use very non-Rust semantics to work with them, it's not surprising (since it's clearly a wrapper type). ScriptInstance
is weird in that Godot gives us direct pointers to it, so I wonder if it should also be some kind of custom wrapper type (which we could then, of course, take by immutable reference).
Experimental branch where ScriptInstance
is completely immutable: https://github.com/TitanNano/gdext/tree/jovan/script_instance_no_mut
I also feel like we have too much direct access to ScriptInstance.
I don't really follow. ScriptInstance
is an abstraction for the GDExtension interface the engine provides / expects to expose a script instance to it. The engine is the only one making calls into an instance, there shouldn't be anyone else holding a reference to it. Where do you see it being passed around?
I would say ScriptInstance
is not that much different from implementing something like IScriptExtension
.
Using &self
everywhere feels a bit like a cop-out: while it makes us no longer need to think about mutability, it pushes the responsibility onto the user, through interior mutability.
Furthermore, it doesn't follow our API design anywhere else -- Array
, Dictionary
etc. also allow &mut self
access, even though they are internally reference-counted. It would also weaken type safety if ScriptInstance::set()
would take &self
: you can no longer pass around a &ScriptInstance
to prevent accidental modification.
I don't really follow.
ScriptInstance
is an abstraction for the GDExtension interface the engine provides / expects to expose a script instance to it. The engine is the only one making calls into an instance, there shouldn't be anyone else holding a reference to it. Where do you see it being passed around?
Let me put it another way. Our ScriptInstance
trait isn't a Godot interface. It's something rust-gdextension
made up. The Godot interface is actually GDExtensionScriptInstancePtr
, which we effectively "subclass" on the Rust side as
struct ScriptInstanceData<T: ScriptInstance> {
inner: RefCell<T>,
gd_instance_ptr: *mut sys::GDExtensionScriptInstanceInfo,
}
And then whenever a method is called on that, we do inner.borrow().whatever()
or inner.borrow_mut().whatever()
. But ScriptInstance
(the Rust-side trait that takes self
) is our invention. So maybe those methods shouldn't be taking &self
or &mut self
at all and should instead be taking either ScriptInstanceData<Self>
or something that resembles it. It still pushes the responsibility for interior mutability onto the user, but it makes it incredibly explicit that that's the case.
It would also weaken type safety if ScriptInstance::set() would take &self: you can no longer pass around a &ScriptInstance to prevent accidental modification.
@Bromeon I think this is a fair point. We would then still have to provide a way to make &mut ScriptInstance
inaccessible (via GodotCell
or otherwise) and integrators of scripting languages would have to work this into their implementation. Every call that is passed from a script to an engine API has to ensure that there is no mutable reference as there is always a change of reentrance and having the constraints of this library leaking into the script language itself is not desirable.
Do you have a suggestion how to approach this instead?
And then whenever a method is called on that, we do
inner.borrow().whatever()
orinner.borrow_mut().whatever()
. ButScriptInstance
(the Rust-side trait that takesself
) is our invention. So maybe those methods shouldn't be taking&self
or&mut self
at all and should instead be taking eitherScriptInstanceData<Self>
or something that resembles it.
@Mercerenies I don't entirely agree with this. ScriptInstance
is a safe version of the GDExtensionScriptInstance
struct. ScriptInstanceData<Self>
is a necessary wrapper to uphold all the safety requirements, and it should not be exposed to the public API in the same way none of the other GDExtension*
types are directly exposed.
Exposing just a cell like &GodotCell<T: ScriptInstance>
definitely would have its advantages, though.
Another option where ScriptInstance
takes a GdCell<Self>
instead of self
. https://github.com/TitanNano/gdext/tree/jovan/script_instance_gd_cell Haven't tested it yet, though.
impl CustomNode { pub fn method(&mut self) { self.ref_gdscript_child_node.call("some_method", &[]); } pub fn on_child_signal(&self) {...} }
func some_method(): self.child_signal.emit()
You can make this work with GodotCell
it just is a bit cumbersome and doesn't come for free:
impl CustomNode {
pub fn method(&mut self) {
let child_node = self.ref_gdscript_child_node.clone();
let guard = self.base_mut();
child_node.call("some_method", &[]);
std::mem::drop(guard);
}
pub fn on_child_signal(&self) {...}
}
@lilizoey Yeah, that's the sort of care that I would like to be possible for people like me implementing a scripting language. Most godot-rust users won't need or care about re-entrancy, so they shouldn't pay the cost of noisy syntax. But it should be possible for those of us who need it. And now, thanks to your work, it is. I hope a similar solution is viable for ScriptInstance
.
@Bromeon regrading your comment in https://github.com/godot-rust/gdext/issues/647#issuecomment-2006462257:
I have so far updated https://github.com/TitanNano/gdext/tree/jovan/script_instance_gd_cell to actually work and extended the itests to include a re-entrance test. This does indeed currently make GdCell
public, something you wrote you would like to avoid. A ScriptInstance
implementation requires access to borrow()
, borrow_mut()
and make_inaccessible()
.
@TitanNano Thanks a lot for looking into it and getting a PoC running! π
Since you've worked with this for a while, you probably know some stuff that I don't and might clarify some things. I'm especially worried about this change:
Which is a considerable downgrade in user-friendlyness, in multiple ways:
this: Pin<&GdCell<Self>>
instead of &self
this.borrow().unwrap().field
instead of self.field
So, a few questions:
call()
was mentioned for recursion.Pin
needed?base()
+ base_mut()
, where calls could be explicitly made re-entrant where needed?Basically, we should not expose GdCell
at all (too low-level), and we should keep the default usage simple. If someone implements ScriptInstance
and doesn't need recursion or other re-entrancy, their life is now suddenly 10 times harder -- how do we justify this? I'd rather provide some power tools where truly needed.
- Is the double-borrow really a problem everywhere? In the initial post of this issue, mostly call() was mentioned for recursion.
Most functions do not execute arbitrary logic defined by script authors. So fn call(...)
would be the most definite case for having re-entrant logic / causing it. get_property
/ set_property
in theory could end up in the same situation if script languages support property setter and getter methods that allow arbitrary calls to other function.
- Why is Pin needed?
Pin is required by GdCell so if we can get it out of the public interface it won't be needed anymore.
- Can we not consider a pattern like
base()
+base_mut()
, where calls could be explicitly made re-entrant where needed?
base()
+ base_mut()
heavily relies upon the Godot inheritance system to gain access to the underlying GdCell
to make the current mutable reference inaccessible. ScriptInstance
on the other hand, uses a struct that is then wrapped by the engine and called into when necessary. Kind of the other way around than the usual inheritance system.
We perhaps could introduce a custom wrapper for &mut self
that is only passed to fn call(...)
(and possibly set_property
and even get_property
) and implements Deref<Self>
. This wrapper could hold a reference to both &mut self
and Pin<&GdCell<Self>>
and expose methods to make the reference inaccessible. This could be implemented as fn owner() / mut_owner()
instead of base
. The script owner / host object is currently something that has to be handled by the ScriptInstance
implementor and should be stored as weak references to avoid creating cyclic references (something I only noticed recently).
Alternatively, we would have to somehow become part of the instantiation of the ScriptInstance
type to store some value inside of it which can later be accessed. This sounds much more complex to me.
(I haven't tested any of this, just some thoughts)
Is this really much simpler of a solution than just making every method take &self
and forcing people to use interior mutability where relevant? both Pin
and GdCell
are rather complicated and require a lot of in depth understanding of rust to use properly, but with &self
the user can decide how intricate they want to be with the interior mutability if they even need it at all.
We perhaps could introduce a custom wrapper for
&mut self
that is only passed tofn call(...)
(and possiblyset_property
and evenget_property
) and implementsDeref<Self>
. This wrapper could hold a reference to both&mut self
andPin<&GdCell<Self>>
and expose methods to make the reference inaccessible. This could be implemented asfn owner() / mut_owner()
instead ofbase
. The script owner / host object is currently something that has to be handled by theScriptInstance
implementor and should be stored as weak references to avoid creating cyclic references (something I only noticed recently).
That sounds interesting, and nicer than a Pin<&GdCell<Self>>
in user signatures. π
Could something like self.owner_mut().some_godot_function()
be used to do re-entrant calls again?
Alternatively, we would have to somehow become part of the instantiation of the
ScriptInstance
type to store some value inside of it which can later be accessed. This sounds much more complex to me.
Not sure what exactly you mean here -- but if you think it's more complex, we can gladly explore the other avenue first π
Is this really much simpler of a solution than just making every method take
&self
and forcing people to use interior mutability where relevant?
Also fair point -- it's a bit hard for me to judge how often one could live with the "basic &self/&mut self
" and when one would need to resort to interior mutability. I had the impression that many typical ScriptInstance
implementations require re-entrant calls, or are these rather special cases?
Thanks a lot for the all the input, it's appreciated!
@lilizoey i agree but it was dismissed in https://github.com/godot-rust/gdext/issues/554#issuecomment-1880734126
Could something like self.owner_mut().some_godot_function() be used to do re-entrant calls again?
In short, yes. The owner of the script instance is just a Gd<T>
that is unrelated to the script instance reference.
I will see if I can get a rough draft of this concept implemented.
At the time I didn't know that the alternative would be so much more complex, to be honest π
So I'm no longer generally against &self
+ interior mutability, but I'd really want to make sure we have considered multiple possible designs first. And we have to approach this from a user-friendliness perspective, not just implementation complexity. An important point here is that such complexity also affects users that do not require re-entrant calls. They don't benefit from it.
To summarize different suggestions:
Everywhere &self
:
Array
, Dictionary
, Gd
etc.&MyScriptInstance
argument doesn't guarantee it's not changed by a functionCustom type Pin<&GdCell<Self>>
:
GdCell
, which is currently an implementation detailself
becomes much more verboseSomething with owner()
+ owner_mut()
Thanks for looking into it @TitanNano. Feel free to keep the PoC small, no need to map the entire ScriptInstance
API initially...
@Bromeon here is the latest POC based on what we discussed: https://github.com/TitanNano/gdext/compare/jovan/script_instance_gd_cell...TitanNano:gdext:jovan/script_instance_mut_wrapper?expand=1
One draw back of this solution is that owner APIs are only available to ScriptInstance
functions which receive the wrapped mutable reference to self
.
I think it'd be possible to more closely mirror GodotClass
here with its base. Maybe do something like
pub trait ScriptInstance {
fn init(owner: Owner<Object>) -> Self;
fn owner_field(&self) -> &Owner<Object>;
fn owner(&self) -> OwnerRef<Object> {
let gd = self.owner_field().to_gd();
OwnerRef::new(gd, self)
}
fn owner_mut(&mut self) -> OwnerMut<Object> {
let owner_gd = self.owner_field().to_gd();
let storage = self.owner_field().get_storage_somehow();
let guard = storage.get_inaccessible(self);
OwnerMut::new(owner_gd, guard)
}
// The rest of the methods
}
Could maybe even split it into two traits, and have a ScriptInstanceWithOwnerField
trait, though i feel like most implementations would want access to the owner field. Worst case you can always just implement owner_field
as unimplemented!()
if you know for sure you'll never use owner()/owner_mut()
.
The API for ScriptInstance
starts to become rather heavy... π€
Now, to implement a custom script extension, the user needs to know:
ScriptInstance
traitcreate_script_instance
functionScriptExtension
classIScriptExtension
trait
instance_create
Owner<T>
typeOwnerRef
+ OwnerMut
types
owner()
+ owner_mut()
methodsGd<T>
or &T
to interact with Godot APIsWhich looks like a very steep learning curve. Sure, there is quite a bit of inherent complexity in the way how script extensions work, but maybe we should also check if/how other bindings implement those and if we could simplify something without abandoning safety (probably not, but worth checking).
Additionally, if we build a parallel Owner<T>
mechanism, we should check if Gd<T>
cannot be reused (in particular the guard types) or at least make sure the duplication is not too big, or localized. In contrast to this API, Gd
has an extreme versatility and can be used all over the place. Owner<T>
would be a highly specialized smart pointer that has no use outside ScriptInstance
.
I'm also a bit worried since there are likely already additional smart pointer types necessary for the multi-threaded case, and a proliferation of those will leave us with gdnative-level complexity that is near-impossible to understand for casual game developers. Again, sometimes complexity is inevitable, and maybe Owner<T>
is indeed the simplest. But let's keep the user perspective in mind here π if things work very similar to Gd
& Co., at least previously learned concepts can be adopted.
as-is, we cannot directly reuse the existing logic from Gd
for doing the same thing with ScriptInstance
. Since the current implementation would require the script instance to derive GodotClass
, as instance storage requires that. But im gonna check if it's possible to generalize the instance storage a little bit to where the instance doesn't need to implement GodotClass
. Then maybe we can just share the existing code between both?
I think that could work, but only if there's enough common code. If we end up needing traits with 2 separate impls, then there's not much code sharing.
Also this would then be more an implementation detail, not the user facing API. But if the latter has conceptual + naming parallels between Owner<T>
and Gd<T>
, that would also help learning.
I think it'd be possible to more closely mirror
GodotClass
here with its base. Maybe do something likepub trait ScriptInstance { fn init(owner: Owner<Object>) -> Self; fn owner_field(&self) -> &Owner<Object>; fn owner(&self) -> OwnerRef<Object> { let gd = self.owner_field().to_gd(); OwnerRef::new(gd, self) } fn owner_mut(&mut self) -> OwnerMut<Object> { let owner_gd = self.owner_field().to_gd(); let storage = self.owner_field().get_storage_somehow(); let guard = storage.get_inaccessible(self); OwnerMut::new(owner_gd, guard) } // The rest of the methods }
Could maybe even split it into two traits, and have a
ScriptInstanceWithOwnerField
trait, though i feel like most implementations would want access to the owner field. Worst case you can always just implementowner_field
asunimplemented!()
if you know for sure you'll never useowner()/owner_mut()
.
A Problem I see with this solution is that you now have taken over the type instantiation of what ever implements the trait which will be quite inconvenient for who ever wants to use ScriptInstance
. GodotClass
solves this with proc-macros and multiple APIs to create a new instance of a type, but any of this seems quite complex for ScriptInstance
.
You also cannot get the script instance storage via the script owner object unless we start maintaining a mapping between godot objects and their script instances. The engine does not expose any way to access script instances.
Additionally, if we build a parallel Owner
mechanism, we should check if Gd cannot be reused (in particular the guard types) or at least make sure the duplication is not too big, or localized. In contrast to this API, Gd has an extreme versatility and can be used all over the place. Owner would be a highly specialized smart pointer that has no use outside ScriptInstance.
I don't see how Owner<T>
could be replaced with Gd<T>
as it basically replicates Base<T>
. Looking at Base<T>
again it seems that the only real difference is that it relies on GodotClass
to access T::Base
. If we can factor the associated base type into a common trait we can reuse the Base
types here.
I updated this with a commit that implements a POC for using Base<T>
, BaseRef<T>
, BaseMut<T>
instead of some custom owner types.
That should probably work yeah, it's a lot less effort than trying to mirror GodotClass
exactly which is a benefit. Using a Base
to represent the owner does sort of make semantic sense actually, because the owner of a script instance is effectively equivalent to the base object of a rust object. In fact, godot even refers to it as a base_object
at least in one place: https://docs.godotengine.org/en/stable/classes/class_script.html#class-script-method-instance-has. We could consider even just naming the methods base/base_mut
?
Needing to add an extra trait in WithBase
is a bit unfortunate but you do need to get a hold of the base type somehow so if we wanna reuse the existing code then that does need to go somewhere, and an extra trait makes sense to me at least.
One thing to consider (which we could do another time too) that i just thought of is: In IScriptExtension::instance_create
the user must always return a pointer created with create_script_instance
. This means that IScriptExtension
must actually be an unsafe trait. Currently instance_create
is an unsafe function, which it doesn't actually need to be afaik since you shouldn't be able to cause UB merely by calling the method. However one way to fix that would be to overwrite instance_create
with our own custom method that has this signature instead:
fn instance_create(&self) -> impl ScriptInstance;
Since then we can just call create_script_instance
when we call this method.
I think this is a bigger issue actually with how we generate bindings currently. Since we just declare any method that takes or returns a pointer unsafe
. But usually methods that return a pointer and doesn't take a pointer doesn't need to be unsafe, but rather the trait itself must be unsafe. I'll look into that a bit more and open a separate issue for that.
If there are no further comments on the topic, I will now start cleaning up the POC and create an PR.
Are there any opinions how the &mut ScriptInstance
ref guard should be named?
Related to #501, though
ScriptInstance
is a bit of a special case.Godot actually passes
ScriptInstance
around asvoid*
for some reason, so on the Rust side we implement a trait calledScriptInstance
whose methods take a&mut self
. In particular, the method of most concern isScriptInstance::call
This method documentation makes the following note.
Which makes any attempts to implement recursion in a Rust-side scripting language a non-starter. I think we can make this method re-entrant, and I'd like to start the discussion on how to do that.
Currently, we implement the GDExtension
ScriptInterface
type asScriptInstanceData<T: ScriptInstance>
, which contains aRefCell<T>
. When we need to invoke a trait function likeScriptInstance::call
, we take theScriptInstanceData<T>
and doThat is, we borrow the contents of the
RefCell<T>
,instance.inner
, for the duration of the call. That makes it impossible to invokecall
again on the same instance recursively from Godot until the original call exits.Proposed Solution 1
My first thought is this.
ScriptInstance
is never used as a trait object, so it needn't be object-safe. So one solution is to change the signature of all of theScriptInstance
methods fromto
Then the implementor of
call
can borrow mutably, decide what it needs to do, and then release the borrow if it's going to make a potentially-recursive call. This just puts the control (and responsibility) of borrowing correctly in the hands of the implementor. I'm not entirely sure how I feel about that, asinstance: RefCell<Self>
is a much more obtuse API than&mut self
, but it definitely would get the job done.Proposed Solution 2
We may be able to apply the
GodotCell
trick toScriptInstance
. I don't claim to fully understand the trickery exhibited by this new cell type, but it looks like it hinges on the fact that theself
in these calls is actually owned by aGd<Self>
. In our case withScriptInstance
, theself
we have is always owned by aScriptInstanceData<Self>
, so a similar trick may be viable.