-
-
Notifications
You must be signed in to change notification settings - Fork 260
Simple API to fetch autoloads #1381
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
Conversation
Previously, custom errors were formatted with Debug impl, which wrapped the error
message in Some("..."). Now properly unwraps the Option and uses Display impl.
cae36aa to
0fcb27a
Compare
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1381 |
0fcb27a to
7aa9305
Compare
Yarwin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine to me, with one caveat – after implementing caching we should check children of the root and pick autoload by class, not the name – making one assumption (there is a node T added as the autoload) instead of two (there is node T named {name} added as the autload). More flexible and less error-prone – traversing scene tree might be expensive when traversed multiple times on runtime, but doing it only once is negligible.
|
How does this deal with name collision? |
|
Example: |
|
I actually raised this issue on discord. Tested it by myself and indeed it doesn't work properly 😅. Sample code: #[derive(GodotClass)]
#[class(init, base = Node)]
struct MyClass {
base: Base<Node>,
}
#[godot_api]
impl INode for MyClass {
fn ready(&mut self) {
let autoload = get_autoload::<MyRustAutoload>();
godot_print!("my autoload: {autoload}");
}
}
#[derive(GodotClass)]
#[class(init, base = Node)]
struct MyRustAutoload {
base: Base<Node>,
}
#[godot_api]
impl MyRustAutoload {
#[func]
fn foo(&self) -> u32 {
44
}
}Godot Editor won't allow us to create autoload with said name:
If we force it via project settings GDScript can't get said value directly as any other autload:
Additionally it just doesn't work in gdscript with static typing: func _ready() -> void:
var some_autoload = get_node("/root/MyRustAutoload")
print(some_autoload.foo())
print("all fine")
# Error: Internal script error! Opcode: 28 (please report).
var my_rust_autoload: MyRustAutoload = get_node("/root/MyRustAutoload")
... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per @ValorZard comment – implement with caching & retrieve autoloads by traversing children of the root node.
|
Is there a way for a rust defined autoload to just not put itself into the Godot namespace so we don’t have name collision? |
7aa9305 to
6c2d12a
Compare
"Global" on/off. https://docs.godotengine.org/en/latest/tutorials/scripting/singletons_autoload.html (that's all it does, adds it to Script Server). I would avoid that though – it would forbid usage of one of the advertised use cases: When you should use an Autoload
Technically no, you can even create new node of the said autoload class – in practice I haven't seen it yet.
We can always move this functionality to external library, which would be invoked by user in build.rs (this is what ttencate suggested recently for generating wrappers for GDScript classes) 🤔. Personally I think "only one instance for autoload (also called a Singleton in Godot) is fine limitation. |
6c2d12a to
c669a93
Compare
|
So it looks like var by_class: AutoloadClass = fetched
assert_eq(by_class.verify_works(), 787, "Autoload typed by class")always works, however the following: var by_name: MyAutoload = fetched
assert_eq(by_name.verify_works(), 787, "Autoload typed by name")only works from Godot 4.3+ onwards. For Godot 4.2, I get: |
godot-core/src/tools/autoload.rs
Outdated
| /// // this returns the one instance of type Gd<GlobalStats>. | ||
| /// let stats = get_named_autoload::<GlobalStats>("Statistics"); | ||
| /// ``` | ||
| pub fn get_named_autoload<T>(autoload_name: &str) -> Gd<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue that in current form it doesn't bring that much improvement over get_node_as<T>("root/xyz") – while being more taxing for Nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll add caching in a 2nd commit once the base one works as expected.
c669a93 to
de635ab
Compare
var by_name: MyAutoload = fetched
assert_eq(by_name.verify_works(), 787, "Autoload typed by name")I think this failing is fine (for GDScript |
de635ab to
cb968c2
Compare
cb968c2 to
5e3c60e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is tempting to over-engineer it with Type::of 😄, but better/more ergonomic shortcuts can be implemented directly by users (for example something along the lines of – fn stats() -> Gd<MyClass> {get_autload_by_name("Statistics")} + pub use stats;) while current approach is flexible enough.
5e3c60e to
573f6c6
Compare
573f6c6 to
a2d23de
Compare
|
Added cleanup logic + test for non-main-thread access.
Yeah, if this becomes a very requested feature, we could even do something like: #[derive(GodotClass)]
#[class(init, base=Node, autoload="Statistics")]
struct GlobalStats { ... }
// Later, through Autoload trait:
GlobalStats::autoload().do_something();But this should be discussed in the context of #1060. |



Closes #126.
Currently no caching yet; we can implement this in a separate PR.