godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.14k stars 93 forks source link

Change the Multi-threaded resource loading API to be more friendly #1430

Open PranavSK opened 4 years ago

PranavSK commented 4 years ago

Describe the project you are working on: This is for any project that needs multi-threaded resource loading and uses GDScript (or any other custom scripting).

Describe the problem or limitation you are having in your project:

Describe the feature/enhancement and how it helps to overcome the problem or limitation: This is with regards to the newly added multi-threaded loading in 4.0 (https://github.com/godotengine/godot/pull/36640#issue-381362185). Currently, this has three functions exposed - one to request a load, one to get the status of load and it's progress, and finally, one to get the loaded resource (or block the main thread till the resource is fully loaded). The main issue with this is that we are additionally needed to poll for the progress or status and also, more importantly, the API feels weird. The polling of status and progress needs to pass an array which is then replaced internally by a single element array. This does not feel very GDScript like (although it is quite common to have return values like this in c++). Also, the use of Array like this is very inconsistent. Many other functions expect a non-empty array while some (like this one) just replace the passed array.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams: The idea is to use an AsyncLoadResult, which is returned when the request is made. Its properties could store the path to the resource and wrap the other 2 functions, namely progress, status and a get_result function. Additionally, it could have a completed signal. This would work great with the new GDScript async.

The current approach:

#...
ResourceLoader.load_threded_request(path)
#...
func _process():
    var prog_arr = []
    var status = ResourceLoader.load_threaded_get_status(path, prog_arr)
    var progress = prog_arr[0]
    #...

The proposed approach:

#...
load_result = ResourceLoader.load_threaded_request(path)
await load_result
#...
# If needed to update the progress:
var status = load_result.get_status()
var progress = load_result.get_progress()
# Also supports - load_result.completed.connect...
# On complete or for wait-to-finish
var resource = load_result.get_resource()

This could potentially improved upon by the user as a multi-threaded task scheduling since in games this is mainly needed for resources (other uses of multi-threaded are probably better handled using an exclusive thread for the system).

If this enhancement will not be used often, can it be worked around with a few lines of script?: Yes, it is possible but feels very redundant.

Is there a reason why this should be core and not an add-on in the asset library?: The idea is to improve the user experience of core API and hence needs to be in the core engine.

Ansraer commented 2 years ago

I personally don't like the idea of having an AsyncLoadResult. This would make things confusing, especially when two different scripts start loading the same resource. You would end up with two different AsyncLoadResult objects. Do both result objects point to the same loaded instance? Or does the same resource get loaded twice? As a developer, I would waste probably waste quite some time double-checking the docs and making sure that I am not unnecessarily duplicating load times every time I use the API. With the current string based approach this feels far more intuitive imo.

With regards to the return parameter, I have to agree that this doesn't really feels like a good solution for gdscript. Something like the following would probably be better:

var status =    ResourceLoader.load_threaded_get_status(path)
var progress =  ResourceLoader.load_threaded_get_progress(path)

Something else I would like to add is that the current method names are quite long and verbose. We could probably make them more compact and readable:

ThreadedResourceLoader.request(path)
ThreadedResourceLoader.get_status(path)
ThreadedResourceLoader.get_progress(path)
ThreadedResourceLoader.get(path)
okla commented 2 years ago

Maybe a simple callback function/lambda can be used?

ResourceLoader.load_threaded(
  "some.tres",
  func(result): do_something(result)
)

Optionally with another progress/status callback

ResourceLoader.load_threaded(
  "some.tres",
  func(result): do_something(result),
  func(progress): print(progress)
)

Or both callbacks may be merged into a single one where request is an array of progress and result (or some new class may be created for the request type)

ResourceLoader.load_threaded(
  "some.tres",
  func(request):
    if request[0] < 100: # progress
      print(request[0])
    else:
      do_something(request[1]) # result
)
Shou commented 1 year ago

I personally don't like the idea of having an AsyncLoadResult. This would make things confusing, especially when two different scripts start loading the same resource. You would end up with two different AsyncLoadResult objects. Do both result objects point to the same loaded instance? Or does the same resource get loaded twice? As a developer, I would waste probably waste quite some time double-checking the docs and making sure that I am not unnecessarily duplicating load times every time I use the API.

Resources are already cached in Godot (at least on 3.x, but I suspect this hasn't changed on master) so they would return the same resource even if you return different AsyncLoadResult objects – if you try to ResourceLoader.load_interactive the same resource in a thread, you'll end up with null returned if you're already loading the resource, so the current behaviour is already to not load the same resource twice. You wouldn't have to reuse the AsyncLoadResult object for the same resource, but you certainly could – I don't think I see a downside to doing that if resources are cached.

Maybe a simple callback function/lambda can be used?

It wouldn't work with await, though, right? Which would be a step back at least when looking at how most languages progressed from callbacks to async/await.

FWIW, the approach described in OP is exactly what I ended up implementing myself in my codebase, so I'll weigh in and say yes please.