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

Provide safe way for implementing IScriptExtension::instance_has #1013

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TitanNano
Copy link
Contributor

After looking into #970 I noticed that there is currently no good way to implement IScriptExtension::instance_has. Godots own implementations of this function rely on some way to access the existing script instances inside the engine.

With the new gdextension function that has been introduced in 4.3 extensions now also have access to the existing script instances in the engine. While it's not safe to access these instance pointers, we can provide safe abstractions for certain use cases. I think this is such a use-case and so far, I haven't identified any other use-case.


There are a couple of necessary changes to enable the integration testing for this new function.

@GodotRust
Copy link

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

@TitanNano TitanNano force-pushed the jovan/script_instance_exists branch from ba7682e to 7d0de74 Compare January 17, 2025 00:30
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.

Thanks a lot!

Maybe general question, is the typical use case to check whether an (obj, script, lang) combination to exist -- but not e.g. to query for a part of the tuple the other valid values?

I've glanced over it, might have a deeper look later 🙂

@TitanNano TitanNano force-pushed the jovan/script_instance_exists branch from 7d0de74 to d2fd93c Compare January 17, 2025 07:58
@TitanNano
Copy link
Contributor Author

Maybe general question, is the typical use case to check whether an (obj, script, lang) combination to exist -- but not e.g. to query for a part of the tuple the other valid values?

Generally the engine creates a script instance inside the objects script setter function, so I don't really see a realistic use case for this yet. So far, the only technical use case I could identify was to easily implement IScriptExtension::instance_has.
For that use case, we only need (obj, script) but object_get_script_instance was implemented to take an object and a ScriptLanguage while the Script class at the same time does not expose the get_language() method.

Regardless of this, we sill should be able to use script_instance_exists in cases where we only have (obj, lang) by using obj.get_script() as the script.

@TitanNano TitanNano force-pushed the jovan/script_instance_exists branch from d2fd93c to a24ca47 Compare January 18, 2025 23:52
@Bromeon Bromeon added feature Adds functionality to the library c: engine Godot classes (nodes, resources, ...) labels Jan 19, 2025
Comment on lines 361 to 364
let object_script: Gd<Script> = object_script_variant.to();
let instance = unsafe { get_instance_fn(object.obj_sys(), language.obj_sys()) };

!instance.is_null() && object_script == script.clone().upcast()
Copy link
Member

Choose a reason for hiding this comment

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

Since 4.4, you can use Variant::object_id():

/// For variants holding an object, returns the object's instance ID.
///
/// If the variant is not an object, returns `None`.
///
/// If the object is dead, the instance ID is still returned. Use [`Variant::try_to::<Gd<T>>()`][Self::try_to]
/// to retrieve only live objects.
#[cfg(since_api = "4.4")]
pub fn object_id(&self) -> Option<crate::obj::InstanceId> {
// SAFETY: safe to call for non-object variants (returns 0).
let raw_id: u64 = unsafe { interface_fn!(variant_get_object_instance_id)(self.var_sys()) };
crate::obj::InstanceId::try_from_u64(raw_id)
}

Since you only want to check for equality, this would save the Variant::to() marshalling, the clone() and the upcast() conversion.

Maybe something like:

Suggested change
let object_script: Gd<Script> = object_script_variant.to();
let instance = unsafe { get_instance_fn(object.obj_sys(), language.obj_sys()) };
!instance.is_null() && object_script == script.clone().upcast()
let instance = unsafe { get_instance_fn(object.obj_sys(), language.obj_sys()) };
let script_id = object_script_variant.object_id().expect(...);
!instance.is_null() && object_script.instance_id() == script_id

I tried backporting Variant::object_id() to <4.4 so you can use it universally, but encountered a bug while doing so. Will follow up on this 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I was able to use upcast_ref() and __object_id() for the comparison, but the Variant::to() marshalling without Variant::object_id() right now.

@TitanNano TitanNano force-pushed the jovan/script_instance_exists branch 2 times, most recently from 4f9fb94 to e183a15 Compare January 20, 2025 23:46
@TitanNano TitanNano force-pushed the jovan/script_instance_exists branch from e183a15 to 5f98714 Compare January 21, 2025 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: engine Godot classes (nodes, resources, ...) feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants