Facepunch / sbox-issues

176 stars 12 forks source link

Whitelist System.Private.CoreLib/System.Delegate.GetInvocationList() #319

Closed peter-r-g closed 7 months ago

peter-r-g commented 3 years ago

What can't you do? Use System.Private.CoreLib/System.Delegate.GetInvocationList()

How would you like it to work? To be able to use this method. To my knowledge there is no issue with it. Just returns all the delegates that are in a delegate array/event so that they could be iterated over or invoked dynamically.

What have you tried?

/// <summary>
/// Requests a map change to take place. If no delegates are registered to OnMapChangeRequested then it will go straight to changing the map.
/// </summary>
/// <param name="map">The map to change to.</param>
public static void RequestChange( string map )
{
    if ( OnMapChangeRequested == null )
    {
        ForceChange( map );
        return;
    }

    ChecksNeeded = OnMapChangeRequested.GetInvocationList().Length;
    if ( ChecksNeeded == 0 )
        ForceChange( map );
    else
    {
        MapChangeRequested = true;
    TimeSinceMapChangeRequested = 0;

    OnMapChangeRequested.Invoke( ( success ) =>
    {
        if ( success )
        {
            ChecksApproved++;
            if ( ChecksApproved == ChecksNeeded )
                ForceChange( map );
        }
        else
        {
            Log.Warning( "Failed to change maps due to a failed delegate" );
            Reset();
        }
    } );
    }
}
github-actions[bot] commented 3 years ago

Stale issue message

peter-r-g commented 3 years ago

But why? It's such a simple thing that would be useful to have access to

peter-r-g commented 3 years ago

Stay away bot

peter-r-g commented 2 years ago

Needs whitelist label added

chrisspieler commented 1 year ago

I find this useful for when I need to perform some expensive calculation to determine whether an event should be invoked. If the length of the array returned by GetInvocationList() is zero, I can skip performing the calculation. The workaround in my case is incrementing/decrementing a "count" property through the event accessors, but the only reason I can do that is because the event is defined in my own code.

LeQuackers commented 1 year ago

Any progress on this?

LeQuackers commented 1 year ago

Bump.

LeQuackers commented 1 year ago

I really don't want to be that guy, but this seems like a really simple whitelist request; is there some underlying issue that's preventing this from being added?

LeQuackers commented 1 year ago

I need this for creating systems where all parties must agree in order for something to happen (e.g. starting a map vote); this is more or less what I'm trying to do:

public class Foo
{
    public event Predicate<FooArgs> CanStart;

    public bool TryStart()
    {
        var args = new FooArgs(this);

        foreach (Predicate<FooArgs> predicate in CanStart.GetInvocationList())
        {
            if (!predicate(args))
            {
                return false;
            }
        }

        Start();

        return true;
    }

    protected virtual void Start()
    {
        // whatever
    }
}

The alternatives for Delegate.GetInvocationList() all suck; whitelisting this would really help me out.

the-real-ale commented 1 year ago

Workaround could be to use the accessors for events to manage your subscriptions.

class Events : IDrawingObject
{
    event EventHandler PreDrawEvent;

    event EventHandler IDrawingObject.OnDraw
    {
        add => PreDrawEvent += value;
        remove => PreDrawEvent -= value;
    }
}

source msdn

To be honest, your application for this seems strange to me without deeper context.

LeQuackers commented 1 year ago

Workaround could be to use the accessors for events to manage your subscriptions.

class Events : IDrawingObject
{
    event EventHandler PreDrawEvent;

    event EventHandler IDrawingObject.OnDraw
    {
        add => PreDrawEvent += value;
        remove => PreDrawEvent -= value;
    }
}

source msdn

To be honest, your application for this seems strange to me without deeper context.

Yeah, the current workaround is to use an accessor to store them in a some collection that you can iterate over later.

In my case each listener registered to event Predicate<FooArgs> CanStart is only ever supposed to get a single vote (i.e. return a bool indicating yes or no). You could make an event EventHandler<FooArgs> CanStart and have the FooArgs record votes using some FooArgs.AddVote(bool) method, but ensuring that every listener actually votes and that every listener only votes once is a giant pain if you do it that way, which is why I opted for event Predicate<FooArgs> CanStart which forces the listener to return a single vote that can be checked in some Delegate.GetInvocationList() loop.

Examples of why event EventHandler<FooArgs> CanStart fails:

public void BadListener1(object sender, FooArgs e)
{
    // Doesn't vote at all; no way to ensure that they actually vote.
}

public void BadListener2(object sender, FooArgs e)
{
    // Casts more than a single vote; no way to ensure each listener only casts a single vote.
    // In the original "event Predicate<FooArgs> CanStart" example this doesn't matter
    // because if there is a single "no" vote, then the "Start" method won't be called;
    // however, if the vote requires 75% of the votes to be "yes", then this would cause problems.
    for (var i = 0; i < 1000; i++)
    {
        e.AddVote(true);
    }
}
yuberee commented 8 months ago

With the new scene system, I feel like this is needed more than ever. Events and delegates are very important for components and GetInvocationList() is very useful. For example I want to await every async task that's subscribed to an event before destroying the gameobject, this way I can have other components attach logic to on death events for a unit in my game without having to expand on the original component or inheriting it which becomes a mess. That is shrimply not possible without GetInvocationList() or workarounds.

yuberee commented 7 months ago

I can confirm it works. Thanks for standing me!