godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Add ability to make unique deferred calls during idle frame #3722

Open Xrayez opened 2 years ago

Xrayez commented 2 years ago

Describe the project you are working on

Goost - Godot Engine Extension

Some existing Goost users find the feature quite useful so I decided to write this proposal.

Describe the problem or limitation you are having in your project

Sometimes (or oftentimes, depending on a problem), I have to make sure that the method is called only once during idle frame. This is because I don't want the same method to be called multiple times which would lead to performance degradation.

I'm talking about dirty flag optimization technique widely used in software engineering, so hopefully there's no need to explain the problem extensively here.

This optimization technique is especially important for procedural generation.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

I propose to implement a mechanism which allows to make unique deferred calls. I see two approaches:

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Typically, the dirty flag optimization is implemented in Godot the following way:

void GradientTexture2D::_queue_update() {
    if (update_pending) {
        return;
    }
    update_pending = true;
    call_deferred("_update");
}

void GradientTexture2D::_update() {
    update_pending = false;

    if (gradient.is_null()) {
        return;
    }
   // Heavy processing goes here...
}

The update could be scheduled via Resource.changed signal emission, which I see as a typical use case for using a dirty flag in Godot context:

void GradientTexture2D::set_gradient(Ref<Gradient> p_gradient) {
    if (gradient.is_valid()) {
        if (gradient->is_connected(CoreStringNames::get_singleton()->changed, this, "_queue_update")) {
            gradient->disconnect(CoreStringNames::get_singleton()->changed, this, "_queue_update");
        }
    }
    gradient = p_gradient;
    if (gradient.is_valid()) {
        gradient->connect(CoreStringNames::get_singleton()->changed, this, "_queue_update");
    }
    _queue_update();
}

Gradient is only an example here, there are many usages of this technique throughout Godot's own codebase.

Here's how it would look like:

func _ready():
    gradient.connect("changed", self, "_on_gradient_changed", [], CONNECT_DEFERRED_UNIQUE)

func _update():
    # Heavy processing goes here...

In contrast to above, this requires much less code (the same amount of code just like for regular calls).

Of course, implementation-wise this won't be as efficient as using the dirty flag technique on a case by case basis, but good enough for most use cases, especially for heavy updates (like in editor).

I believe this is something that must be implemented in the MessageQueue C++ singleton in Godot. In 4.x, using Callables will make this efficient enough in order to avoid duplicate calls during idle time.

If this enhancement will not be used often, can it be worked around with a few lines of script?

On the case by case basis, this can be implemented via script, but that's far from a few lines of code, and you have to do this every time. I'd like to avoid excess code duplication in my project.

Is there a reason why this should be core and not an add-on in the asset library?

There are several reasons why this must be implemented in core:

  1. This is something which complements Object functionality.
  2. It's not trivial to implement via script (if not using the case-by-case approach).
  3. Performance issues using GDScript. This can be implemented in C++ as well, but unlike in GDScript, it's really difficult to add singletons/autoloads from within C++ modules (in order to flush deferred calls each frame).

Note that some of this is already implemented in Goost via GoostEngine.defer_call_unique().

KoBeWi commented 2 years ago

Something similar was discussed on the chat: https://chat.godotengine.org/channel/core?msg=vLiEN4BEM9M8TziDw (you need to scroll down a bit, because RocketChat message links suck)

Xrayez commented 2 years ago

Nice, I didn't expect it, I'm going to link Akien's message here then: https://github.com/godotengine/godot/pull/53767#issuecomment-942285516.

Regarding whether this should be implemented on a signal level or method, I think it's just a matter of front-end implementation, so both can be supported.

If you're not sure whether it's possible to implement, you can look at implementation at Goost, could likely be optimized if needed, ideally should be implemented in MessageQueue (I've had to copy-paste some implementation from there):

As I said, indeed it may not be as efficient as using a boolean flag, but I don't see a strong reason for not implementing this feature either, moreover it would be likely much more easier to implement using Callable in 4.x.

MessageQueue singleton itself could be exposed for advanced use cases and/or convenience (removes the need to add a dedicated singleton just for this):

func _ready():
    resource.connect("changed", MessageQueue, "call_deferred_unique", ["_update"])

func _update():
    # Heavy processing goes here...

If you think that this will lead to confusion and misuse, the Godot's API already has potential problem in this regard (I don't see this as a problem personally as long as features get properly documented), Like, we do have find_node() vs get_node() (see already closed proposal #1303). Similarly, we could have both call_deferred_unique() and call_deferred(): the first will always be slower, but it doesn't mean it's not useful. 🙂

The engine could keep using boolean flags internally for everything. Similarly, Node.find_node() is only used once or twice in 3.x in core, but it's still there because it's widely useful for game projects.

Regardless of whether core developers decide to implement it or not, I invite people to use Godot + Goost builds if you feel like it's a pattern that you use often, along with other useful methods such as invoke(): goostengine/goost#52.

nathanfranke commented 2 years ago

I support this. Keywords: debounce, debouncing even though those aren't exactly the right words, that's the first thing I searched.


func _print(text): print(text)

func _ready():
    _print.bind("a").call_deferred_unique()
    _print.bind("b").call_deferred_unique()
    _print.bind("a").call_deferred_unique()

What should this print? Either ab or ba is acceptable, but should be deterministic.

My use case for why b then a would be helpful I have a hierarchy of nodes like this: ``` → Game → Map → Slot1 → Slot2 → Slot3 ``` Each node listens to enter/exit tree and calls signals about the state change. I am listening to these changes in my UI (pseudocode): ``` on map changed: update map texture on slots changed: update slots, requires map to be up to date ``` To improve performance, I am debouncing the signal (what this proposal suggests). ``` map added to tree, set to [Map] slot added to tree, append [Slot1] slot added to tree, append [Slot2] slot added to tree, append [Slot3] (debounced map signal emitted) (debounced slots signal emitted) --- now i change the map --- slot removed from tree, erase [Slot3] slot removed from tree, erase [Slot2] slot removed from tree, erase [Slot1] map removed from tree, set to [null] map added to tree, set to [NewMap] slot added to tree, append [NewSlot1] slot added to tree, append [NewSlot2] slot added to tree, append [NewSlot3] ``` Finally back to the `ab` `ba` example, this will either give: ``` (debounced slots signal emitted) (debounced map signal emitted) ``` or ``` (debounced map signal emitted) (debounced slots signal emitted) ``` . The former is what I got with my tentative implementation, which was undesired, as the map wasn't yet updated. I preferred the latter in my use case.

And some workarounds (you're welcome):

extends Node

signal hello(i)

func _ready() -> void:
    if "you want the first emission to pass through (ab in example)":
        var queue := []
        hello.connect(func(i):
            queue.push_back({})
            (func():
                if not queue.is_empty():
                    queue.clear()
                    print("First debounced with ", i)
            ).call_deferred()
        )

    if "you want the last emission to pass through (ba in example)":
        var queue := []
        hello.connect(func(i):
            queue.push_back({})
            (func():
                queue.erase({})
                if queue.is_empty():
                    print("Second debounced with ", i)
            ).call_deferred()
        )

    for i in 5:
        hello.emit(i)

prints:

First debounced with 0
Second debounced with 4