Cancelled LoadFromUri return true
Describe the bug
When calling "GltfImport.Load()" and cancelling the task before it has finished to download the file, the method return true even if the model has not been loaded.
The method documentation states:
<returns>True if loading was successful, false otherwise</returns>
which is not respected in this case.
To Reproduce Steps to reproduce the behavior:
- Create a new MonoBehaviour with this code:
using GLTFast;
using GLTFast.Logging;
using System.Threading;
using UnityEngine;
public class DebugCancellation : MonoBehaviour
{
private async void Start()
{
GltfImport Importer = new GltfImport(null, null, null, new ConsoleLogger());
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource();
cancellationTokenSource.Cancel();
bool success = await Importer.Load("C:/foo", null, cancellationTokenSource.Token);
Debug.Log($"[{Time.frameCount}] success: {success}");
}
}
- Add this script to a GameObject in your scene
- Enter playmode
- See returned success is
true
Expected behavior
The method returns false because the model hasn't been loaded (file at path C:/foo doesn't exist on your computer).
Desktop (please complete the following information):
- glTFast version 6.14.1
- Unity Editor version 6000.0.43f1
Additional context
Issue come from here:
https://github.com/atteneder/glTFast/blob/a77908be91ee0fdb6007e789aeed51f80580039a/Runtime/Scripts/GltfImport.cs#L1173
I don't understand why it returns true. I would have changed it to false. I wouldn't have changed it to success variable defined just above, because when the file exists, the download is successful but the glb hasn't been successfuly loaded because of the early return.
I think true should indicate successful completion without critical errors (note, missing textures are not critical).
I also think false should indicate unsuccessful completion. That leaves two options:
- Throw an exception
- Return a state enum that can have more than two options.
I think option 1 would be the best-practice C# way, but I'll have to test how this works across platforms.
Thanks for raising this to us.