Skip to content
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

Proposal: Optimize notifications. #693

Open
CedNaru opened this issue Sep 20, 2024 · 0 comments
Open

Proposal: Optimize notifications. #693

CedNaru opened this issue Sep 20, 2024 · 0 comments

Comments

@CedNaru
Copy link
Member

CedNaru commented Sep 20, 2024

Notification methods in Godot are a special case that don't work like the rest of the API. They are implemented in the core layer of the engine, and scripts and extensions have their own api to handle them, unlike regular registered methods.

The reason for that design is because of the dual nature of _notification().
Most Godot methods fall into 2 categories:

  • The non-virtual methods that will be generated as part of the API and called directly by the scripts. Godot already implemented their behaviour, and they are not supposed to be overwritten afterward.
  • The virtual methods. They are the reverse of the previous type. They are supposed to be implemented by users and Godot calls them as part of its regular lifecycle callbacks (_enter_tree(), _process()) or in specific situation.

_notification is actually both at the same time. A lot of Godot classes already have an implementation for it, but their children can overide them as well as script.
What is peculiar about it is that Godot is using macro black magic and method pointers to call the whole inheritance chain of implementation. On top of that, those methods can be called in parent to child order or child to parent order (depending on whether the notification is supposed to be initializing or destroying something, it's the same logic as the order of constructors and destructors for classes).

Because of those requirements, we can't implement notifications like regular methods in Godot. We tested and it doesn't seem possible to call a method in a non-virtual/polymorphic way in Kotlin (Meaning that if class B inherit A, we can't call A's implemention on B if it overrides it).
And we can't ask users to use super() either because the actual call order also depends on the reverse argument, which would lead to weird and error-prone implementation.

Right now the trick is using the following syntax:

    @RegisterFunction
    override fun _notification() = godotNotification { notification: Int ->
          when(notification) {
           // Your code
          }
    }

It works well. Behind the scene, we are never calling that function during the game execution. Instead, we call it at the very beginning ,for each script, and retrieve a lambda from it. Later, that list of lambda is then executed in natural or reverse order depending on the reverse argument.

The main issue comes with the way Godot uses notifications and the cost of JNI calls. Once you implement that method for any scripts or parent in your inheritance tree, Godot is going to call that method for every single notification it receives. The engine has dozens of kinds of notifications split between many different classes. And Nodes being organized in a tree structure, they propagate the notifications they receive to all their children. This generates a lot of JNI calls if you happen to have a Node implementing that method.

Most of the time, those calls will be useless because an object only cares about a very small amount of notifications. The method will simply be called without any computation being actually done. Ideally, we don't want to make a JNI call if we know the call won't do anything.

I then propose to change the way notifications are implemented by users by making them register the notification that must be processed as well.

I am not sure of what syntax would be best but I have several suggestions:

Same as today, but we return a list of GodotNotification alongside the notification integer:

    @RegisterFunction
    override fun _notification() = listOf(
          godotNotification(NOTIFICATION_ENTER_TREE) { // Your code }
          godotNotification(NOTIFICATION_EXIT_TREE) { // Your code }
    )
    

Individual methods with new annotation:

    @RegisterNotification
    fun notification1() = godotNotification(NOTIFICATION_ENTER_TREE) { // Your code }
    @RegisterNotification
    fun notification2() = godotNotification(NOTIFICATION_EXIT_TREE) { // Your code }
    
    // OR
    
    @RegisterNotification(NOTIFICATION_ENTER_TREE)
    fun notification1() = godotNotification { // Your code }
    @RegisterNotification(NOTIFICATION_EXIT_TREE)
    fun notification2() = godotNotification { // Your code }

Static methods:

object: GodotNotification<YourScript> {
   fun notification1() = godotNotification(NOTIFICATION_ENTER_TREE) { // Your code }
   fun notification2() = godotNotification(NOTIFICATION_EXIT_TREE) { // Your code }
}

With that kind of design, we can send to the C++ the list of notification supported and only make a JNI call when it's a match, avoiding easily hundreds/thousands of JNI call per frame if a common node with a JVM script need notifications.

@CedNaru CedNaru changed the title Proposal: Optimize notifications. Proposal: optimize notifications. Sep 20, 2024
@CedNaru CedNaru changed the title Proposal: optimize notifications. Proposal: Optimize notifications. Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant