godot-rust / gdext

Rust bindings for Godot 4
https://mastodon.gamedev.place/@GodotRust
Mozilla Public License 2.0
3.06k stars 190 forks source link

Can't preload resources #360

Closed LeaoLuciano closed 6 days ago

LeaoLuciano commented 1 year ago

In GDscript preload() loads resource when script is parsed.

image

But in Godot Rust there's no way to preload a resource. I suggest adding a preload attribute to GodotClass derive macro:


#[derive(GodotClass)]
#[class(base=Node)]
pub struct Example {
    #[preload("/path")]
    texture: Gd<ImageTexture>,
    #[base]
    base: Base<Node>,
}

So during the class registration, the resource is loaded (maybe using Godot's ResourcePreloader class).

Bromeon commented 1 year ago

In GDscript preload() loads resource when script is parsed. But in Godot Rust there's no way to preload a resource.

How do you imagine this to work? The Godot engine is not available at gdext compile time (see also https://github.com/godot-rust/gdnative/issues/400).

At startup/registration time it's possible, but what would the difference between ResourcePreloader and a call to load::<T>() in ready() be?

The syntax for easy loading has been suggested before (https://github.com/godot-rust/gdnative/issues/980) and is probably a separate discussion from load/preload 🙂

mhgolkar commented 1 year ago

I think as @Bromeon said, we need a syntax for easy loading more than preloading. And I should add it's probably better in most of common cases to use dynamic / interactive resource loader in background, such as (pre-)loading everything under a custom/animated boot screen and share them between classes that use them. Background loading when your home-screen/main-menu or intro scene is being played can work really nice and smooth for resources you'll going to use a lot and globally.

But with all the introduction, if someone really needs the functionality, a build script may be the answer. It's a "may be" because it should be thought first in terms of practicality. A build script can crawl code to find hints where you need preloaded entities, then create a .gd class/script with one const per preloading resource (maybe with exports) and a mapping method to easily share them using their path as key. The generated class/script would be set to be imported (in Godot editor as a singleton) and used to get references to the resources in runtime (from GDScript or Rust code). A macro like preload!("res://foo/bar.tres") can be used as both hint for the build script, and to be replaced with something like: godot::engine::Preload::singleton().share("res://foo/bar.tres").expect("preloaded resource existing in {path}") on compile, where obviously the share method returns an option including shared (or shallow copied) reference to the resource.

Disclaimer! I still think interactive loading may be a better approach in most of cases!


Now considering the easy loading matter: It would be great if init as in #[class(init, base=...)], could expand (_ready) for resource loading, as well as fetching references to the nodes in the tree (using relative or absolute node paths). Do you think I should open an issue to propose it?

Bromeon commented 1 year ago

Thanks for the input!

But with all the introduction, if someone really needs the functionality, a build script may be the answer. It's a "may be" because it should be thought first in terms of practicality.

A big issue is that Godot is currently not required for compiling a library with gdext (this is to allow clean programming against a given Godot API, independent of installed/runtime versions). And we would need the engine to parse resource files.

But as you say, I haven't heard a good argument for preloading in Rust that goes beyond "GDScript has it". Loading at startup (without build scripts) is something that should be possible in Rust though.

Background loading when your home-screen/main-menu or intro scene is being played can work really nice and smooth for resources you'll going to use a lot and globally.

Background means separate thread. While nice from a UX perspective, it needs a bit of thinking because resources are not thread-safe during loading (see thread-safety guidelines). It needs even further thought regarding how to map this soundly to Rust's threading/safety model.

Do you think I should open an issue to propose it?

I think it's good here, as it's a general discussion about loading ergonomics. Feel free to post examples. I can adjust the issue title later if needed.

Also, please read existing discussions I linked on the topic. We have a tendency to reinvent wheels in gdext 😉

mhgolkar commented 1 year ago

A big issue is that Godot is currently not required for compiling a library with gdext ...

I didn't look at it that fancy! The build script would only create one .gd file (extending node) in the build target directory and the rest happens as if the user adds another script to the Godot's singleton system quite manually. There is just need to somehow get that singleton in rust too, which in worse case scenario can happen by adding the generated class to the scene tree like any other node and fetch it by rust like any other node and call its share method. Yet as we agree there has been no good argument for it.

Background means separate thread... It needs even further thought regarding how to map this...

Yes sure! I suggested it as a general design advice, not using Rust GDExt necessarily. Right now one can use a hook/workaround in which when a quite heavy resource (e.g. a whole scene change) is needed, a call to a GDScript bridge class queues it to load using existing Godot's infrastructure. The bridge checks for the state of resource readiness, avoiding the process to be blocked, and when the resource is ready, it feeds the resource to the rust (via callback, etc.). In other words loading happens outside the rust and is delegated to Godot itself which handles multi-threading and more. It is indeed to keep things smooth from UX point of view and also allows to update a progress indicator on each readiness check as a bonus! Although a little tricky, I have tried it and it works.

mhgolkar commented 1 year ago

Regarding the easy loading, I think a little syntax sugar on top of what already exists can help with ergonomics. Quite similar to what @LeaoLuciano suggested, it starts with adding few attributes:

#[derive(GodotClass)]
#[class(init, base=Node)]
pub struct Catcher {
    #[base]
    base: Base<Node>,
    #[catch(res="res://resources/fish.svg", hint="ImageTexture", cache="CACHE_MODE_REUSE")]
    fish: Gd<Texture2D>,
}

Which expands as if we had implemented it ourselves:

fn init(base: Base<Control>) -> Self {
    let fish = {
        godot::engine::ResourceLoader::singleton()
            .load_ex("res://resources/fish.svg".into())
                .type_hint("ImageTexture".into())
                .cache_mode(godot::engine::resource_loader::CacheMode::CACHE_MODE_REUSE)
                .done()
            .expect("required `ImageTexture` resource from 'resources/fish.svg'")
        .cast::<Texture2D>()
    };
    // ...
    Self {
        base,
        fish
    }
}

And if it was fish: Option<Gd<Texture2D>> instead, it could not expect and map the loaded resource to cast.


In terms of getting reference to the other nodes, I guess, things get a little tricky, because I'm not sure if there is a safe way to access scene tree in that stage. I suggest either let init to also implement _enter_tree and _ready if possible to get reference to other nodes, or create a (naturally one-shot) listener in init stage for another one (e.g. ready) calling another automatically implemented method that will fetch the nodes safely.

#[derive(GodotClass)]
#[class(init, base=Node)]
pub struct Parent {
    #[base]
    base: Base<Node>,
    #[fetch("./foo/bar")] // or even "%global_node"
    child: Option<Gd<Control>>,
}

It will expand as if we had implemented this ourselves:


#[godot_api]
impl Parent {

    #[func]
    fn _fetch_on_ready(&mut self) {
        self.child = Some( self.base.get_node_as::<Control>("./foo/bar") );
    }

}

#[godot_api]
impl NodeVirtual for Parent {

    fn init(mut base: Base<Node>) -> Self {
        let base_ref = base.share();
        base.connect(
            "ready".into(),
            Callable::from_object_method(base_ref, "_fetch_on_ready")
        );
        // ...
        Self {
            base,
            child: None,
        }
    }

}

These are for convenience of course. Although one may argue if it is necessary, more convenient or more performant, comparing to getting the reference each time we need it. On the other hand it helps with corner cases when a node is going to move from one parent to another: as far as I remember, you can not get the same node with its path again (naturally), but a held reference remains valid. Anyway, finding a solution to get rid of the Option, would make the convenience even more attractive, if there is a safe/possible way to achieve that.

The init could even do initialize children, if we really push it further! Imagine something like this:

#[derive(GodotClass)]
#[class(init, base=Node)]
pub struct Parent {
    #[base]
    base: Base<Node>,
    #[add(use_bbcode=true, text="[color=yellow]crab![/color]")]
    child: Gd<RichTextLabel>,
}

Which expands to:

fn init(mut base: Base<Node>) -> Self {
    let mut child = RichTextLabel::new_alloc();
    child.set("use_bbcode".into(), Variant::from(true));
    child.set("text".into(), Variant::from("[color=yellow]crab![/color]"));
    base.add_child(child);
    // ...
    Self {
        base,
        child
    }
}

Another wishful example of pushing init forward:

#[derive(GodotClass)]
#[class(init, base=Node)]
pub struct Parent {
    #[base]
    base: Base<Node>,
    #[add(internal="INTERNAL_MODE_FRONT")]
    #[set(use_bbcode=true, text="[color=yellow]crab![/color]")] // (when `#[add]` exists)
    #[connect(signal="gui_input", handler="_on_child_gui_input", flag="CONNECT_DEFERRED")] // (handler in parent)
    child: Gd<RichTextLabel>,
}

Sky is the limit, you know!

Bromeon commented 1 year ago

We should not push too much into proc-macro APIs -- while convenient, they have several disadvantages:

Some of your proc-macro examples use the equivalent amount of code as if those fields were initialized in ready(), so is there really much benefit?

Also, anything late-initialized cannot have type Gd<T> as that pointer does not have a null state.

mhgolkar commented 1 year ago

So weighing disadvantages against conveniences how further we may expect init to go? (asking as an end user)

I think it does what it's supposed to do already, so nothing further is necessary. Are there any more conveniences you are contemplating? I can imagine more possibilities off the top of my head, like automatically implementing initializer that accepts optional struct fields of the extension type and returns GD<Self> (e.g. Parent::new_from( child: GD<...>, ...) -> Gd<Parent>). None of these are urgent though. So keeping things to minimum also makes complete sense, considering all the important work with much higher priority that is there to be done! What is your general approach?

Bromeon commented 1 year ago

What is your general approach?

There's no formal set of rules, but I tend to prioritize roughly in this descending order:

Don't quote me on that though 😉 I don't follow such a list, it's just roughly how I see things and it's subject to change all the time. In general, the priority of an issue is mainly decided by two factors:

  1. How many users are affected by it?
  2. How much effort is the implementation and future maintenance?

The 2nd point also depends on contributors, and if there is already a design that could work. I am rather conservative in accepting massive changes in a single PR, because the chances that things fit well into existing systems are lower. Also, people are often eager to contribute but forget that every line of code has to be maintained for an indefinite amount of time, so there's a cost to every addition as well. Sometimes, other core parts of the library need to be ironed out before a new feature is ready.

I hope that clarifies things a bit 🙂

Bromeon commented 1 year ago

So weighing disadvantages against conveniences how further we may expect init to go? (asking as an end user)

I have no concrete plans yet -- there is already #[init(default)], but it may still change as well.

Something like the @onready equivalent may be interesting as well, but mostly for node paths and not arbitrary expressions.

Whether a feature is worth a dedicated proc-macro API depends on the individual use case and the motivation behind it. Over time, we should probably establish some clearer criteria w.r.t. what is in scope and what isn't. As you say, the sky is the limit, but ultimately gdext aims to be pragmatic for everyday gamedev usage, and APIs should be as simple as possible without restricting more advanced usages. Of course that's very general again 😀

mhgolkar commented 1 year ago

It all sounds wise.

I think platform support there tries to climb up in the list of priorities, specially regarding the ever growing mobile market. Current state of everything else may be considered ready enough for some projects, but when export time arrives, the platform support can scare some away! Of course there is always the issue of man power, time and resources.

Anyway! You and GDExt contributors have done a great job. I really appreciate it and wish you all health and happiness.

bluenote10 commented 1 year ago

How to best approach the resource loading problem is also something I've been wondering about. I'm not sure if there are significant performance differences between "script-based preloading" or simply loading them at runtime in Rust, but I'd expect loading them in Rust is fine.

My initial approach was to manually load a bunch of resources at once within _ready in one of the "fundamental root nodes" in the node tree, and then passing them down into sub-components as needed. However, the passing around got a bit tedious.

Currently I'm experimenting with having one central resource.rs which gives access to "lazy static" (and thread local) instances of resources. Something like that:

use godot::engine::ShaderMaterial;
use godot::prelude::*;
use once_cell::unsync::Lazy;
use std::sync::Mutex;

// Since the Godot thread safety guidelines specify that loading resources isn't thread
// safe, I'm guarding the loading with a mutex.
static LOADING_MUTEX: once_cell::sync::Lazy<Mutex<()>> =
    once_cell::sync::Lazy::new(|| Mutex::new(()));

pub fn shader_material_foo() -> Gd<ShaderMaterial> {
    thread_local! {
        static INSTANCE: Lazy<Gd<ShaderMaterial>> = Lazy::new(|| {
            let _lock = LOADING_MUTEX.lock();
            try_load::<ShaderMaterial>("shaders/foo.tres").unwrap()
        });
    }
    INSTANCE.with(|instance| instance.share())
}

pub fn shader_material_bar() -> Gd<ShaderMaterial> {
    thread_local! {
        static INSTANCE: Lazy<Gd<ShaderMaterial>> = Lazy::new(|| {
            let _lock = LOADING_MUTEX.lock();
            try_load::<ShaderMaterial>("shaders/bar.tres").unwrap()
        });
    }
    INSTANCE.with(|instance| instance.share())
}

// ... many more resources (could use a macro to reduce the boilerplate of the pattern above)
pub fn shader_material_baz() -> Gd<ShaderMaterial> {
    // ...
}

// I call the `preload_resources` from a central place at start-up to preload a selected
// subset of resources. This allows to relatively easily switch from preloading to lazy
// loading a certain resource.
pub fn preload_resources() {
    shader_material_foo();
    shader_material_bar();
    // shader_material_baz() // let's lazy load this one...
}

Not quite sure if that's a good idea though :wink:

EDIT: An obvious flaw of the current implementation is that it doesn't drop the resources at the process exit, leading to "RID allocations ... were leaked at exit" warnings. Perhaps with a bit more fancy wrapping it would be possible though to come up with a manual release_resources() method that could be called at an Predelete notification.


Regarding the side discussion on @onready and getting references to the node tree: Isn't this mostly independent from resource loading?

Bromeon commented 1 year ago

Not quite sure if that's a good idea though 😉

One thing I really don't like about Rust is how excessively boilerplaty the code becomes as soon as anything global is involved. I mean I understand why some of it is needed, and it's probably good to discourage reckless use of globals (especially with multi-threading), but... some valid use cases are still very hard to model safely.

For example, if you have a "loading phase" where the globals are initialized, and a "using phase" where all the access is read-only. All your data types still need to account for late-initialization, locking, etc. The only way to avoid this is UnsafeCell or static mut... I know that OnceLock intends to do something like that, but it's on a per-variable basis, more complex workflows don't work.

That was a bit off-topic... In terms of resource loading, there is maybe potential to find some patterns that can be abstracted away, so that gdext users don't need deeply nested synchronization types.

Regarding the side discussion on @onready and getting references to the node tree: Isn't this mostly independent from resource loading?

Yeah, probably makes sense to look at those as orthogonal issues.

bluenote10 commented 1 year ago

One thing I really don't like about Rust is how excessively boilerplaty the code becomes as soon as anything global is involved.

Absolutely, especially for occasional Rust users like me!

This would be a revised version of the thread-local-lazy-static pattern that doesn't leak resources by offering an explicit cleanup. Just demonstrating it for a single resource here:

thread_local! {
    static INSTANCE_RESOURCE_FOO: Mutex<RefCell<Option<Gd<ShaderMaterial>>>> = Mutex::new(RefCell::new(None))
}

// The lazy resource getter:
pub fn get_resource_foo() -> Gd<ShaderMaterial> {
    INSTANCE_RESOURCE_FOO.with(|mutex| {
        let ref_cell = mutex.lock().unwrap();
        let mut option = ref_cell.borrow_mut();
        let instance = match option.as_ref() {
            None => {
                let _lock = LOADING_MUTEX.lock();
                // Actual loading functionality goes here:
                let instance = try_load::<ShaderMaterial>("shaders/line_basic.tres").unwrap();
                *option = Some(instance.share());
                instance
            }
            Some(instance) => instance.share(),
        };
        instance
    })
}

// Corresponding cleanup function:
pub fn release_resource_foo() {
    INSTANCE_RESOURCE_FOO.with(|mutex| {
        let ref_cell = mutex.lock().unwrap();
        let mut option = ref_cell.borrow_mut();
        *option = None;
    })
}

And my scene tree root node takes care of calling a release_resources function that releases all resources via an on_notification:

    fn on_notification(&mut self, what: ControlNotification) {
        if what == ControlNotification::Predelete {
            release_resources();
        }
    }

The challenge with static + thread_local seems to be that it is not possible to drop the underlying type explicitly. That's why I decided to switch from once_cell::unsync::Lazy to Mutex<RefCell<Option<...>>> -- more or less re-implementing a lazy but moving the Option from the outside to the inside, so that I can have control over dropping the value myself.


However, the fact that manual cleanup calls are necessary raises the question if statics are really the way to go. If the resources would be tied to the lifetime of some ResourceHolder node in the scene tree, proper cleanup would be automatic. However this then raises the question how to get easy+safe access to that node. Something like get_resource_holder().get_resource_foo() would be nice, but registering a node reference in a singleton is either tricky or just plain dangerous. In any case, approaches in this direction could be related to getting references to the node tree after all...

Bromeon commented 9 months ago

Now that OnReady<T> exists (https://github.com/godot-rust/gdext/pull/534), and as we can't exactly have GDScript-like preload semantics, is there still demand for this?

If yes, we should discuss what concretely is missing.

mhgolkar commented 9 months ago

is there still demand for this?

Speaking for myself, I think your valuable time is better invested on other fronts. (personally, the part I really feel missing right now is a standard/convenient way to call GDScript funcs from Rust and getting the returned values, which I'm sure you have in your mind already.)

Bromeon commented 9 months ago

(personally, the part I really feel missing right now is a standard/convenient way to call GDScript funcs from Rust and getting the returned values, which I'm sure you have in your mind already.)

Thanks for the input!

Indeed "external APIs" have been discussed for quite a while, most recently in https://github.com/godot-rust/gdext/issues/372 🙂 I assume you think of something type-safe and auto-generated? Maybe I should open a dedicated proposal issue once I've made some more thoughts on the design...

mhgolkar commented 9 months ago

Well, yes, that... and... I am more interested in ability to invoke custom GDScript methods defined in .gd files (maybe through call()) somehow like gdnative's function calls. If I'm not missing something, we can not have such calls right now. I guess they more or less have the same underlying system though.

Bromeon commented 9 months ago

I am more interested in ability to invoke custom GDScript methods defined in .gd files

Yes, we're talking about that too in #372 🙂

If I'm not missing something, we can not have such calls right now.

You can have them through Object.call, just dynamic.

mhgolkar commented 9 months ago

Oh! my bad! Yes that works perfectly fine!