Cysharp / R3

The new future of dotnet/reactive and UniRx.
MIT License
2.32k stars 103 forks source link

ReactiveProperty<T>.SetValue() is missing? #77

Closed sakano closed 10 months ago

sakano commented 10 months ago

I'm currently trying out R3 by replacing UniRx in my existing Unity project.

The project has a class that inherits from ReactiveProperty\ as follows. In R3, I cannot implement this because there is no way to set currentValue from a child class.

/// <summary>
/// ReactiveProperty whose set value is clamped in the range of [min, max].
/// </summary>
public sealed class ClampedReactiveProperty<T> : ReactiveProperty<T> where T : IComparable<T>
{
    private readonly T min, max;

    public ClampedReactiveProperty(T initialValue, T min, T max) : base(initialValue)
    {
        this.min = min;
        this.max = max;
    }

    // XXX: I cannot override this in R3!!!
    protected override void SetValue(T value)
    {
        if (Comparer.Compare(value, min) < 0) base.SetValue(min);
        else if (Comparer.Compare(value, max) > 0) base.SetValue(max);
        else base.SetValue(value);
    }

    private static IComparer<T> Comparer { get; } = Comparer<T>.Default;
}

By the way, do you plan to implment abstract ReactiveProperty\ to substitute UniRx's IReactiveProperty\? It would allow me to implement my own ClampedReactiveProperty even if ReactiveProperty\.SetValue() is missing.

My project contains several classes that implement IReadOnlyReactiveProperty\ and I have successfully replaced them all with ReadOnlyReactiveProperty\. Is there any reason why there is no abstract ReactiveProperty\?

neuecc commented 10 months ago

Thank you, I completely forgot to include the equivalent functionality. I will add it. Since using SetValue would lead to a comparison with the EqualityComparer, which is not desirable, I plan to choose a different method name that is close to it.

Increasing unnecessary inheritance hierarchies leads to complexity, so I do not intend to add more base classes. For example, if there were something like ReactivePropertyBase, when creating a method that takes a ReactiveProperty, there would be a dilemma about whether the argument should be ReactivePropertyBase or ReactiveProperty.

neuecc commented 10 months ago

I've released v0.1.19, it include ReactiveProperty.OnValueChanging(ref T value).

public sealed class ClampedReactiveProperty<T>(T initialValue, T min, T max)
    : ReactiveProperty<T>(initialValue) where T : IComparable<T>
{
    private static IComparer<T> Comparer { get; } = Comparer<T>.Default;

    protected override void OnValueChanging(ref T value)
    {
        if (Comparer.Compare(value, min) < 0)
        {
            value = min;
        }
        else if (Comparer.Compare(value, max) > 0)
        {
            value = max;
        }
    }
}

// pattern of no primary-constructor
public sealed class ClampedReactiveProperty2<T>
    : ReactiveProperty<T> where T : IComparable<T>
{
    private static IComparer<T> Comparer { get; } = Comparer<T>.Default;

    readonly T min, max;

    // callOnValueChangeInBaseConstructor to avoid OnValueChanging call before min, max set.
    public ClampedReactiveProperty2(T initialValue, T min, T max)
        : base(initialValue, EqualityComparer<T>.Default, callOnValueChangeInBaseConstructor: false)
    {
        this.min = min;
        this.max = max;

        // modify currentValue manually
        OnValueChanging(ref GetValueRef());
    }

    protected override void OnValueChanging(ref T value)
    {
        if (Comparer.Compare(value, min) < 0)
        {
            value = min;
        }
        else if (Comparer.Compare(value, max) > 0)
        {
            value = max;
        }
    }
}