Instance creation now goes through proper placeholder process #1597
Conversation
`init_stage_test.rs`: classes `SomeObject` and `MainLoopCallbackSingleton`. If substituted as placeholders, `bind()` would panic.
Cover `Gd`'s placeholder behavior for runtime classes in the editor, regarding: * default/cached property values (`get`, `set`, property list, class identity) * panic in `bind()` * virtual callbacks (skipped for placeholders)
afd41af to
1410d09
Compare
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1597 |
1410d09 to
47d0abe
Compare
|
Looks good. I also tested it with my project, and it didn't break any of my tool classes. 👍 Maybe we provide a deprecated alias for |
|
Oh, very good catch, I missed that we publicly re-export this. I'll add it back, we can always change in the future. |
| panic!( | ||
| "Gd::{method}() called on a placeholder instance of `{name}`.\n\ | ||
| A runtime (= non-tool) class does not have a real instance in the editor.\n\ | ||
| Use `#[class(tool)]`, or guard with `Engine::singleton().is_editor_hint()`.", |
There was a problem hiding this comment.
| pub(crate) fn panic_placeholder_bind<T>(method: &str) -> ! { | ||
| panic!( | ||
| "Gd::{method}() called on a placeholder instance of `{name}`.\n\ | ||
| A runtime (= non-tool) class does not have a real instance in the editor.\n\ |
There was a problem hiding this comment.
We don't use "runtime class" anywhere in public-facing docs. Can we use tool class instead, like earlier? In the past you raised argument that it is implementation detail which might not be obvious/intuitive to user (since Godot doesn't really use it anywhere)
There was a problem hiding this comment.
Fair, I changed it to "non-tool class".
Maybe we can at some point introduce "runtime class" as a concept, but it should probably be in a more prominent place, not some isolated error messages.
Replace `AtomicBool` with `AtomicEnum`. The "Unknown" state applies mostly for Godot < 4.4 && level < Scene.
47d0abe to
41cb5ae
Compare

Fixes #1404.
Fixes #912.
This PR changes
Gd::default_instance()so it goes through the editor/placeholder process when needed.I decided against blanket call to
ClassDb::instantiate(), because this downgrades an indirect C function call (createcallback) to a Godot reflection call with all the bells and whistles + extraVariantmarshalling. I didn't want to have this overhead on hot paths in games, as object default-creation is still a pretty fundamental operation.Instead, the code first checks whether editor is running, and only then goes through
ClassDb::instantiate(). For non-editor runs, the old way works fine, so we keep using that.Checking whether editor is running turns out to be harder than I thought, and takes up half of this PR (but contained in last commit). The problem is that
Engineclass isn't available in init-stage <Scenefor Godot < 4.4, and I've found no other easy way thanEngine::is_editor_hint()to check this. So we need to have a tristate (editor, runtime, unknown).Apart from this, the PR adds quite a few itests around placeholder behavior. Thanks to #1591, we can now test those and will be notified immediately if we break something or Godot changes semantics.
I would use this PR as a base for future improvements, e.g. no longer implying
toolin#[class(singleton)].