Redot-Engine / redot-proposals

Redot Improvement Proposals
MIT License
33 stars 8 forks source link

Revise StringNames and NodePaths in C#. #64

Open Joy-less opened 2 weeks ago

Joy-less commented 2 weeks ago

Describe the project you are working on

A 3D MMORPG with lots of moving parts and dependencies, each of which needs to be very performant.

Describe the problem or limitation you are having in your project

StringNames and NodePaths, which were clearly designed for GDScript, cause garbage collection performance spikes [1, 2] in C#. Since they're used intermittently and are implicitly casted from strings, they can be easy to miss. Functions that use them are often called every frame (e.g. Input.IsActionPressed(StringName Action)), making these issues very insidious. Even the Godot and Redot Docs overlook this.

There are several workarounds:

  1. Store StringNames in variables.

    static readonly StringName Position = "position";
    // ...
    Get(Position);

    This solution is cumbersome and messy when you have a lot of StringNames.

  2. Use the source-generated cached StringNames.

    Get(Node3D.PropertyName.Position);

    This solution only works for properties and methods of built-in classes. It doesn't work for shader parameters, GDScript interop, and the various methods that use them (for example Input and Tween and MultiplayerSynchronizer).

  3. Use caching functions

    Get(StringName("position"));

    This is my personal solution since it's the cleanest. (See code). However, it pretty much invalidates the whole purpose of StringNames and NodePaths which is to improve performance.

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

The ideal solution would be one of the following:

  1. Remove StringNames and NodePaths, since they don't add much value. This would be ideal, and clearly works in other engines like Unity. However, it could be a lot of work and would be a breaking change.

  2. Remove StringNames and NodePaths from the C# API. This would also be ideal, since they are such an issue in C# specifically. GDScript uses reference counting which avoids the garbage collection spikes. However, it would also be a breaking change.

  3. Use caching in the implicit casts to StringName/NodePath. This would be a simple patch that would also solve the issue. StringNames and NodePaths could still be cached as variables if you needed the performance boost (but realistically 99% of games don't need it since it's a micro-optimisation).

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

Assuming the 3rd solution is chosen (to use caching in the implicit casts) here are my caching functions:

global using static RecycleExtensions;

using ZiggyCreatures.Caching.Fusion;

public static class RecycleExtensions {
    private static readonly FusionCache StringNameCache = new(new FusionCacheOptions());
    private static readonly FusionCache NodePathCache = new(new FusionCacheOptions());

    /// <summary>
    /// Converts the string to a StringName, recycling if possible.
    /// </summary>
    public static StringName StringName(string String) {
        return StringNameCache.GetOrSet(String, _ => (StringName)String);
    }
    /// <summary>
    /// Converts the string to a NodePath, recycling if possible.
    /// </summary>
    public static NodePath NodePath(string String) {
        return NodePathCache.GetOrSet(String, _ => (NodePath)String);
    }
}

The StringName implicit cast currently looks like this:

public static implicit operator StringName(string from)
{
    return new StringName(from);
}

Instead, it could be changed to this (replace FusionCache with your cache of choice):

using ZiggyCreatures.Caching.Fusion;

private static readonly FusionCache StringNameCache = new(new FusionCacheOptions());

public static implicit operator StringName(string from)
{
    return StringNameCache.GetOrSet(from, _ => new StringName(from))!;
}

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

The enhancement should be regularly used. Please see my discussion in the Godot Engine repository to see the performance difference. The workarounds are simple but can be easy to miss and are overcomplicated.

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

It is not possible for this to be an add-on, since you can't monkey-patch in C#.

Joy-less commented 2 weeks ago

Please see my discussions in the Godot and Redot repository for further reading.

Spartan322 commented 3 days ago

Remove StringNames and NodePaths, since they don't add much value.

This won't happen, it violates compatibility.

Remove StringNames and NodePaths from the C# API.

This neither can happen, it both violates compatibility and violates binding functionality.