Skip to content

Document runtime class in editor requirement #1168

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions godot-core/src/classes/class_runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,30 @@ pub(crate) fn ensure_object_inherits(derived: ClassName, base: ClassName, instan
)
}

#[cfg(debug_assertions)]
pub(crate) fn ensure_binding_not_null<T>(binding: sys::GDExtensionClassInstancePtr)
where
T: GodotClass + Bounds<Declarer = bounds::DeclUser>,
{
if !binding.is_null() {
return;
}

// Non-tool classes can't be instantiated in the editor.
if crate::classes::Engine::singleton().is_editor_hint() {
panic!(
"Class {} -- null instance; does the class have a Godot creator function? \
Copy link
Member

Choose a reason for hiding this comment

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

Newline would be nice:

Suggested change
"Class {} -- null instance; does the class have a Godot creator function? \
"Class {} -- null instance; does the class have a Godot creator function?\n\

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought so, but it weirdly splits the warning into two which is a bit too confusing:

image

Copy link
Member

Choose a reason for hiding this comment

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

Hm, but we already use multiple lines in many other error messages. Then we should rather fix how it's represented in Godot error printing (outside this PR), no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That definitively makes sense

Ensure that the given class is a tool class with #[class(tool)], if it is being accessed in the editor.",
std::any::type_name::<T>()
)
} else {
panic!(
"Class {} -- null instance; does the class have a Godot creator function?",
std::any::type_name::<T>()
);
}
}

// ----------------------------------------------------------------------------------------------------------------------------------------------
// Implementation of this file

Expand Down
36 changes: 26 additions & 10 deletions godot-core/src/obj/raw_gd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -425,8 +425,19 @@ where
}
}

// TODO: document unsafety in this function, and double check that it actually needs to be unsafe.
unsafe fn resolve_instance_ptr(&self) -> sys::GDExtensionClassInstancePtr {
/// Retrieves and caches pointer to this class instance if `self.obj` is non-null.
/// Returns a null pointer otherwise.
///
/// Note: The returned pointer to the GDExtensionClass instance (even when `self.obj` is non-null)
/// might still be null when:
/// - The class isn't instantiable in the current context.
/// - The instance is a placeholder (e.g., non-`tool` classes in the editor).
///
/// However, null pointers might also occur in other, undocumented contexts.
///
/// # Panics
/// In Debug mode, if binding is null.
fn resolve_instance_ptr(&self) -> sys::GDExtensionClassInstancePtr {
if self.is_null() {
return ptr::null_mut();
}
Expand All @@ -437,16 +448,21 @@ where
}

let callbacks = crate::storage::nop_instance_callbacks();
let token = sys::get_library() as *mut std::ffi::c_void;
let binding = interface_fn!(object_get_instance_binding)(self.obj_sys(), token, &callbacks);

debug_assert!(
!binding.is_null(),
"Class {} -- null instance; does the class have a Godot creator function?",
std::any::type_name::<T>()
);
// SAFETY: library is already initialized.
let token = unsafe { sys::get_library() };
let token = token.cast::<std::ffi::c_void>();

// SAFETY: ensured that `self.obj` is non-null and valid.
let binding = unsafe {
interface_fn!(object_get_instance_binding)(self.obj_sys(), token, &callbacks)
};

let ptr: sys::GDExtensionClassInstancePtr = binding.cast();

#[cfg(debug_assertions)]
crate::classes::ensure_binding_not_null::<T>(ptr);

let ptr = binding as sys::GDExtensionClassInstancePtr;
self.cached_storage_ptr.set(ptr);
ptr
}
Expand Down
4 changes: 3 additions & 1 deletion godot-macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,9 @@ use crate::util::{bail, ident, KvParser};
/// See [`ExtensionLibrary::editor_run_behavior()`](../init/trait.ExtensionLibrary.html#method.editor_run_behavior)
/// for more information and further customization.
///
/// This is very similar to [GDScript's `@tool` feature](https://docs.godotengine.org/en/stable/tutorials/plugins/running_code_in_the_editor.html).
/// This behaves similarly to [GDScript's `@tool` feature](https://docs.godotengine.org/en/stable/tutorials/plugins/running_code_in_the_editor.html).
///
/// **Note**: As in GDScript, the class must be marked as a `tool` to be accessible in the editor (e.g., for use by editor plugins and inspectors).
///
/// ## Editor plugins
///
Expand Down
Loading