Document runtime class in editor requirement #1168
Conversation
Yarwin
added
documentation
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1168 |
| if crate::classes::Engine::singleton().is_editor_hint() { | ||
| panic!( | ||
| "Class {} -- null instance; does the class have a Godot creator function? \ | ||
| Ensure that given Class is runtime class (#[class_tool]) if it is being accessed in the editor.", |
There was a problem hiding this comment.
| Ensure that given Class is runtime class (#[class_tool]) if it is being accessed in the editor.", | |
| Ensure that the given class is a tool class with #[class(tool)], if it is being accessed in the editor.", |
| // Non-runtime 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? \ |
There was a problem hiding this comment.
Newline would be nice:
| "Class {} -- null instance; does the class have a Godot creator function? \ | |
| "Class {} -- null instance; does the class have a Godot creator function?\n\ |
There was a problem hiding this comment.
I thought so, but it weirdly splits the warning into two which is a bit too confusing:
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
That definitively makes sense
godot-core/src/obj/raw_gd.rs
Outdated
| let binding = interface_fn!(object_get_instance_binding)(self.obj_sys(), token, &callbacks) | ||
| as sys::GDExtensionClassInstancePtr; |
There was a problem hiding this comment.
Please avoid as casts, they're the crowbar of the type system.
(I should probably look for clippy lints at some point...)
Use <*mut T>::cast() instead, maybe in a separate variable.
| return; | ||
| } | ||
|
|
||
| // Non-runtime classes can't be instantiated in the editor. |
There was a problem hiding this comment.
You mean "non-tool classes", no?
- Add safety note. - Make `resolve_instance_ptr` safe. Returning (and casting) null ptrs is totally safe.
godot-core/src/obj/raw_gd.rs
Outdated
| // SAFETY: library is already initialized. | ||
| let token = unsafe { sys::get_library() }; | ||
| // SAFETY: ensured that `self.obj` is non-null and valid. | ||
| let binding = unsafe { | ||
| interface_fn!(object_get_instance_binding)(self.obj_sys(), token.cast(), &callbacks) | ||
| }; |
There was a problem hiding this comment.
Please empty lines before "groups" of comment+code:
| // SAFETY: library is already initialized. | |
| let token = unsafe { sys::get_library() }; | |
| // SAFETY: ensured that `self.obj` is non-null and valid. | |
| let binding = unsafe { | |
| interface_fn!(object_get_instance_binding)(self.obj_sys(), token.cast(), &callbacks) | |
| }; | |
| // SAFETY: library is already initialized. | |
| let token = unsafe { sys::get_library() }; | |
| // SAFETY: ensured that `self.obj` is non-null and valid. | |
| let binding = unsafe { | |
| interface_fn!(object_get_instance_binding)(self.obj_sys(), token.cast(), &callbacks) | |
| }; |
But more importantly, type inference for dangerous pointer conversions like .cast() is really an antipattern in my opinion. It also bears the risk that if the signature changes, then our conversion semantics are silently updated, which may be unintended. Not a big deal here because GDExtension APIs are stable, but still... Naming the type ensures that our mental model is correct.
Could you declare token with the explicit converted-to type, e.g.:
// SAFETY: library is already initialized.
let token = unsafe { sys::get_library() };
let token = token.cast::<???>();There was a problem hiding this comment.
If we are going to establish a convention, I would go with
let variable: T = ptr.cast();due to type aliases (as far as I'm aware .cast::<*#$%>!@#(&!!()-| (choose the right one or none) TypeAlias>() is not an option 🤔? I've never seen it)
example:
(...)
pub type GDExtensionClassInstancePtr = *mut __GdextClassInstance;
// Silly – `sys::__GdextClassInstance` is not mentioned anywhere else, and we can't cast directly to `sys::GDExtensionClassInstancePtr`.
let ptr: sys::GDExtensionClassInstancePtr = binding.cast::<sys::__GdextClassInstance>();There was a problem hiding this comment.
Makes sense here! But there are also cases where you have the pointee type and would need to write an extra *mut or *const around it, if used on the left-hand-side.
We should probably just use whichever is easier, main thing is that a type is involved somewhere 🙂
- Explicitly declare type while `cast()`ing, so type inference won't try anything funny
- Code tweaks – improve readability when specifying a type we cast our pointer to
Closes: #755 (comment)
Document requirement of
#[class(tool)], the error will inform user about given requirement (identical to https://github.com/godotengine/godot/pull/94511/files)