readyplayerme / rpm-unity-sdk-core

This Module contains all the core functionality required for using Ready Player Me avatars in Unity, including avatar loading and creation
MIT License
93 stars 30 forks source link

Timing issues on LoadAvatarAsync vs OnCompleted #269

Closed ripperdoc closed 5 months ago

ripperdoc commented 6 months ago

Describe the bug When I load multiple avatars, sometimes LoadAvatarAsync finishes before OnCompleted event is called. This results in racing conditions where the avatar sometimes isn't ready to use even if all async loading tasks seem completed.

Files

This is not specific to any specific avatar URL. Cannot share project code at this point.

To Reproduce

Roughly this code:

    private void Start()
    {
        rpmLoader = new AvatarObjectLoader();
        rpmLoader.OnCompleted += StorePreloadedAvatar;
    }

    private void StorePreloadedAvatar(object sender, CompletionEventArgs completionEventArgs)
    {
        Debug.Log($"{completionEventArgs.Url} OnCompleted");
    }

    public async Task<LoadResult> LoadRpmAvatar(string url)
    {
        Debug.Log($"{url} before LoadAvatarAsync");
        var rpmResponse = await rpmLoader.LoadAvatarAsync(url);
        Debug.Log($"{url} after LoadAvatarAsync");
    }

    public async void StartLoading()
    {
        await Task.WhenAll([
            LoadRpmAvatar("https://models.readyplayer.me/6523a41a7137f30e3a522e4a.glb"),
            LoadRpmAvatar("https://models.readyplayer.me/64ef0b3542c59d7dceb2f468.glb"),
            LoadRpmAvatar("https://models.readyplayer.me/656db9a9165ecd09deacd570.glb")
        ]);
    }

This will sometimes output this log:

https://models.readyplayer.me/6523a41a7137f30e3a522e4a.glb before LoadAvatarAsync
https://models.readyplayer.me/64ef0b3542c59d7dceb2f468.glb before LoadAvatarAsync
https://models.readyplayer.me/656db9a9165ecd09deacd570.glb before LoadAvatarAsync
https://models.readyplayer.me/6523a41a7137f30e3a522e4a.glb OnCompleted
https://models.readyplayer.me/6523a41a7137f30e3a522e4a.glb after LoadAvatarAsync
https://models.readyplayer.me/64ef0b3542c59d7dceb2f468.glb after LoadAvatarAsync
https://models.readyplayer.me/656db9a9165ecd09deacd570.glb after LoadAvatarAsync
https://models.readyplayer.me/64ef0b3542c59d7dceb2f468.glb OnCompleted
https://models.readyplayer.me/656db9a9165ecd09deacd570.glb OnCompleted

Expected behavior

I expect the loading to be in order, so that OnComplete always fires before LoadAvatarAsync finishes. The fix is to create a separate behaviour to ensure all OnComplete events have been called, but that basically means I have no use for LoadAvatarAsync and could just load it will traditional callback / Coroutine style code.

Desktop (please complete the following information):

HarrisonHough commented 5 months ago

Hey thanks for reporting.

When it comes to loading multiple avatars, we recommend creating separate avatar loaders because in general, it's not always the case that the first one requested is going to be the first one completed. One of the reasons is that some avatars have bigger file size depending on what assets are equipped. But the other thing is that on our backend, we do some caching to improve request times. EG if its the very first time ever requesting an avatar, it will take longer is it might need to bake the latest changes, but if you requested the same avatar 1 hours ago, then it would still be in the cache and no need to rebake it.

There is also the SDK local avatar caching that we do (if you enable it). If enabled it will download and store the avatar .glb locally, so naturalyl if you request it again it doesn't need to redownload so the avatar will load faster.

In any case I would recommend adjusting your code a little like this.

    private void StorePreloadedAvatar(object sender, CompletionEventArgs completionEventArgs)
    {
        Debug.Log($"{completionEventArgs.Url} OnCompleted");
    }

    public async Task LoadRpmAvatar(string url)
    {
        var rpmLoader = new AvatarObjectLoader();
        rpmLoader.OnCompleted += StorePreloadedAvatar;
        Debug.Log($"{url} before LoadAvatarAsync");
        var rpmResponse = await rpmLoader.LoadAvatarAsync(url);
        Debug.Log($"{url} after LoadAvatarAsync");
    }

    public async Task StartLoading()
    {
        await Task.WhenAll(
            LoadRpmAvatar("https://models.readyplayer.me/6523a41a7137f30e3a522e4a.glb"),
            LoadRpmAvatar("https://models.readyplayer.me/64ef0b3542c59d7dceb2f468.glb"),
            LoadRpmAvatar("https://models.readyplayer.me/656db9a9165ecd09deacd570.glb")
        );
    }

If you want to ensure sequential loading you could as you mentioned just use the non async method. We have an example of this with couroutines in our Loading Samples https://github.com/readyplayerme/rpm-unity-sdk-core/blob/main/Samples~/AvatarLoadingSamples/MultipleAvatarLoading/MultipleAvatarLoadingExample.cs

Let me know if you have any further questions.

ripperdoc commented 5 months ago

Hi, thanks for the quick response!

To clarify, yes, I want to load multiple avatars in parallel and I expect them to be of different sizes and therefore finish at different times, but for each of those avatars I expected that the OnCompleted event is sent at the same time as LoadAvatarAsync returns, not that the event comes later sometimes. This led to bugs, as the avatars were not completely ready to use even LoadAvatarAsync had returned.

To create a separate loader for each avatar seem to work, so I can use that, thanks! I guess it comes down to semantics - I thought a loader should be re-used for many loads (ex share resources) and I though OnComplete coincides with every LoadAvatarAsync completing. (in fact, if the avatar was 100% loaded when LoadAvatarAsync returned, I wouldn't need to use OnComplete at all).

HarrisonHough commented 5 months ago

No problem. It is an interesting use case, I guess you could say it is a limitation with the current implementation of our async loading method. For us the simplest way to solve this was to go with a separate loader for each avatar request. However, if we see a significant need for it to be different we may revisit this in the future, it's not in our current roadmap though.

That's an interesting point about the need for OnComplete on an the async method and something I actually didn't think about. I think the only reason why it's there is because our first versions of the loader used this approach. I will bring this up with the team though, perhaps it makes sense to make a version that just return awaits the avatar data rather then requiring the use of the callbacks.

Thanks for the insights.