[Map Instance] Expose LoadMapAsync

Well you can do something like this:

public static class DelegateExtensions
{
  public static Task Await( this FakeClass t )
  {
    var result = new TaskCompletionSource();

    Action eventAwaiter = null;

    eventAwaiter = () =>
    {
      result.SetResult();
      t.onComplete -= eventAwaiter;
    };

    t.onComplete += eventAwaiter;

    return result.Task;
  }
}

public class FakeClass
{
  public Action onComplete;
}

public class TimerTester : Component
{
  private FakeClass t;

  [Button( "Await event." )]
  private async void AwaitFakeClass()
  {
    await t.Await();
    Log.Info("T invoked!");
  }
}

(And before anyone says anything you for whatever reason cannot do:

t.onComplete.Await();
//and
  public static Task Await( this Action action )

The Action seemed to be boxed into a new reference, so the original reference is unaffected by operations done on it.)

But I'm confused why you don't just do something like:

public class MapLoader : Component
{
  [Property] private MapInstance map;
  public static bool Loading { get; private set; } = true;

  protected override void OnAwake() {
    map.OnMapLoaded += StartGame;
  }

  private void StartGame() {
    //Do some setup actions
    Loading = false;
  }

  [Broadcast( NetPermission.HostOnly )]
  public void ChangeMap ( string s ) {
    Loading = true;
    map.MapName = s;
  }
}

//Then if you have other code you want to wait during loading you can just do:
protected override void OnUpdate() {
  if( MapLoader.Loading ) return;
}

Obviously it depends on your setup, but it seems like less of a headache than making the event a task. Plus you can have a static [HostSync] dictionary which keeps track of which players are loaded:

    [HostSync] public NetDictionary<Guid, bool> Dictionary { get; set; } = new();

    protected override void OnFixedUpdate()
    {
        var clientsLoaded = Scene.GetAllComponents<Client>();
        foreach ( var client in clientsLoaded ) {
            if( Dictionary.ContainsKey(client.Id))
            {
                if( Dictionary[client.Id] != client.Loaded)
                    Dictionary[client.Id] = client.Loaded;
            }
        }

        //If anyone disconnects
        foreach ( var item in Dictionary.Keys.Where( x => !clientsLoaded.Any( y => x == y.Id ) ) )
        {
            Dictionary.Remove( item );  
        }
        }

Then the players that are loaded can see a view of the map with a scoreboard showing the loading state of the other players.

Obviously there's more cool code variations you can do rather than just that but to check if everyone's loaded you'd just do:

[HostSync] public static bool EveryoneIsLoaded { get; set; } = false;

protected override void OnFixedUpdate()
{
  //Code from earlier
  EveryoneIsLoaded = Dictionary.Keys.All( x => Dictionary[x] );
}

Or you could add some logic so you're not broadcasting the new value of EveryoneIsLoaded every frame, but yeah!

I guess I'm confused on what's limiting you, since the way things are currently implemented doesn't stop you from waiting until everyone is loaded, you just need to add some control logic to your classes, but you have to do that anyway and I'm not sure how awaiting in one specific area would make it easier.

Why exposing it might be a bad idea

Facepunch can expose LoadMapAsync, but often times with async methods the more strictly their invocation is controlled the less error prone they are.

For example, since it's only ever invoked interally, MapName is always set before it's invoked. If they just exposed it without changing the method at all, next frame the MapName would be different than the loadedMapName, and the old map would load. Simple enough to fix:

private async Task LoadMapAsync(string mapName)
{
  MapName = mapName;

But this then adds another variable that might cause a race conditon.

Right now the only things that can invoke LoadMapAsync is OnLoad, which is called once, and OnUpdate which is called once per frame. While it's possible for these to overlap, it's unlikely for it to happen unintentionally, maybe if you had two classes, and the second class's OnLoad happens after MapInstance, which then changes MapName, so on MapInstance's OnUpdate it tries to load a different map, something like that. Like it's theoretically possible but unlikely.

Similarly, since there's not a lot of code before hitting the semaphore, and the method is invoked once a frame at most, it's unlikely the few variables that could be succeptible to race conditions will be.

But if you do:

map.LoadMapAsync("a");
map.LoadMapAsync("b");
map.LoadMapAsync("c");

You'll have no idea which map will load.

Granted if you had several components editing MapName in the same frame you might also not be sure which map loads, but it'll always be the same component that takes priority and it's easier to check which order OnUpdate is called on all your components, as opposed to figuring out what instructions are being called in what order on async methods.

The only other ways you could have inconsistent behaviour is editing NoOrigin and invoking UnloadMap() at weird times. And I'm not sure UnloadMap would cause weird exceptions because I think using (Scene.Push()) acts like a semaphore?

So practically speaking I think the only race condition that can happen right now is if you changed NoOrigin? Everything else is either locking or has too long between invocations to be an issue.

But if they exposed LoadSceneAsync and wanted it to be error proof they'd probably have to mess around with the class and alter some of the functionality, maybe adding another semaphore, making all the externally accessible variables atomic, whatever.

It's probably not particularly hard but you kinda have to "fix" something that isn't broken.

Facepunch / sbox-issues

[Map Instance] Expose LoadMapAsync #5790

For?

What can't you do?

How would you like it to work?

What have you tried?

Additional context

Why exposing it might be a bad idea