Add AsyncPCK support (ResourceLoader.load_threaded_* version)#116673
Add AsyncPCK support (ResourceLoader.load_threaded_* version)#116673adamscott wants to merge 4 commits intogodotengine:masterfrom
ResourceLoader.load_threaded_* version)#116673Conversation
|
High level note: the additional (and extremely lengthy) descriptions on these functions are IMO a good indication that these should be separate functions, even if they do a lot of similar things under the hood. Especially the state within a state with the download and load phases is particularly gross API design. |
AR-DEV-1
left a comment
There was a problem hiding this comment.
LGTM. There are some issues here & there but they'll be fixed.
|
I think there's a risk that the simplified API becomes more cumbersome to use and has more pitfalls that might be hard to notice, the necessity to use Based on what I can tell is the workflow in the original PR the way to do general loading would be largely unchanged for your basic actions and usage, with some additional API calls to handle invoking loading of async resources While I haven't tested either implementation in practical use I feel that having just some dedicated fetch and install code running specifically on Web with minimally intrusive code and then just loading as usual for all practical load code with no additional adjustments feels less complex and intrusive than adding new API for this, especially if there's a requirement to adjust to use threaded loading operations universally for those cases. The threaded load operation would take a bit of overhead to use to replace any normal And I agree with the above that the need to clarify this is a good argument for dedicated API |
|
This is looking good - is it possible to add an API for getting the amount of downloaded bytes? The big issue that this initiative solves is players churning during load times - lower load times are obviously the best case (less time for the players to get bored and leave!) but a crucial addition is visible progress. If the player can see what's going on/how long is left, particularly with a solid number that they can see ticking upwards, they are less likely to churn. |
The current PR do tell the download progress using |
e20e318 to
f426f49
Compare
|
Added @mihe and @syntaxerror247 as reviewers as I am touching the way platform code is shared. |
| const HashSet<String> &get_dependencies() const { | ||
| return dependencies; | ||
| } |
There was a problem hiding this comment.
This obviously isn't my area of expertise, but since this comment was left unresolved in #114690, this seems worth bringing up again.
Last I heard there was quite the fundamental problem with having GDScriptParser::get_dependencies return GDScript dependencies like this, since ResourceLoader requires (?) that resource dependencies form an acyclical graph, whereas GDScript can have cyclical dependencies.
Is there something about the changes to ResourceLoader in this PR, or the way dependencies is populated, that addresses that concern? If not, then this change would presumably end up reopening issues like #91726 again, unless they've been mitigated in some other way since then.
Given what you said about this whole PR falling apart without this change it seems like this would be worth clearing up.
There was a problem hiding this comment.
I'd suggest to just add an additional get_cyclic_dependencies method for the time being. We can add a default impl that just returns get_dependencies and GDScript can only implement the later. Systems from which we explicitly know that they were written with cyclic dependencies in mind can utilize the information without the risk of breaking existing stuff.
There was a problem hiding this comment.
That would add get_cyclic_dependencies() for all Resources, and IMHO, is redundant. Especially since Resources cannot have cyclic dependencies with exceptions.
There was a problem hiding this comment.
Especially since
Resources cannot have cyclic dependencies with exceptions.
I mean that's really up to the resource format loader at the end of the day, isn't it?
Alternatively we can also double check that every usage of get_dependencies does support cycles (maybe you did?).
As far as I'm aware there is no concrete issue with cycles just a general uncertainty on whether they are sufficiently supported (but if someone knows of some concrete reports please correct me). The previous PR was reverted due to a regression which does not stem from dependency cycles as far as I understood it.
There was a problem hiding this comment.
Let's leave concerns about support for cyclic references aside for a second.
I'm not convinced that tracking deps in the analyzer is a valid approach. Our other resource format loaders take special care not to actually load dependencies in get_dependencies. And previous regressions clearly show that current users are relying on this behaviour (If someone wants to play detective: 519fce9).
Best we can do with that restraint is doing stuff in the parser. (I don't think anyone is keen on adding more behaviour flags to the analyzer?)
There was a problem hiding this comment.
Best we can do with that restraint is doing stuff in the parser. (I don't think anyone is keen on adding more behaviour flags to the analyzer?)
But the parser doesn't have any concept of files other than itself, if I remember well.
There was a problem hiding this comment.
Fair, but where you place it codewise isn't really the issue. The analyzer would probably be fine. More like that I don't think we should load any external files for obtaining dependencies (from my understanding that caused the regression which lead to reversion the last time).
I don't think we would need to load them either. A soon as you would need to load an external file, you have your dependency and just skip ahead. Basically like island parsing but for the analyzer if that makes sense.
There was a problem hiding this comment.
Thanks!
I haven't been able to test, because the PR won't compile right now, but I personally really like the high-level direction!
I think using the threaded resource API is the right choice, because it is meaningful on all platforms. I suspect most Godot apps with a web build aren't web-exclusive, but run on multiple platforms, and I worry if this was only exposed as a web-specific API, that no one would actually use it. Using the threaded resource loader for loading levels can provide a smoother experience on desktop, and now that will be true for the web too, just for different reasons.
I've only had a chance to skim the changes, and there's a lot here, so I'll probably need to take a 2nd pass later. :-) I'll do that when I can also test it
KoBeWi
left a comment
There was a problem hiding this comment.
I don't have setup for Web build, so I can't test this in action, but I did a code review.
Looks like this PR includes some refactoring and minor enhancements. Such changes shouldn't be part of big feature PRs, unless they are minor codestyle changes (like the gdscript.h change here), or necessary for the feature (although in that case, if they can exist separately, a separate PR is better). At the very least they should be in separate commits. Are the EdtiorExportPlatform changes necessary though? (i.e. extracting some methods/enums to new files)
The API changes (which are none xd) are fine; in the future we might want to expose the new OS methods, depending on feedback. It would allow some greater flexibility in installing files.
There was a problem hiding this comment.
These don't seem to be used anywhere?
And even then, adding SceneStringNames to existing identifiers should also involve replacing existing usages, so ideally it should be a separate PR.
There was a problem hiding this comment.
I'll remove the definitions!
| using DebugFlags = EditorExportPlatformData::DebugFlags; | ||
| using ExportMessageType = EditorExportPlatformData::ExportMessageType; | ||
| using ExportMessage = EditorExportPlatformData::ExportMessage; |
| if (r_error != nullptr) { \ | ||
| *r_error = (m_err); \ | ||
| } \ | ||
| ((void)0) |
There was a problem hiding this comment.
What is the ((void)0) thing for?
There was a problem hiding this comment.
It ensures that a ; is needed after the macro use.
The compiler will want the semi-column, but will essentially remove the ((void)0) code as this is a no-op.
| if (data->native_file.exists && !data->remap_file.exists) { | ||
| resource_file = &data->native_file; | ||
| } else if (data->remapped_file.exists) { | ||
| resource_file = &data->remapped_file; |
There was a problem hiding this comment.
| if (data->native_file.exists && !data->remap_file.exists) { | |
| resource_file = &data->native_file; | |
| } else if (data->remapped_file.exists) { | |
| resource_file = &data->remapped_file; | |
| if (data->remapped_file) { | |
| resource_file = &data->remapped_file; | |
| } else if (data->native_file.exists) { | |
| resource_file = &data->native_file; |
There was a problem hiding this comment.
data->remapped_file is always true, it's not a pointer.
| } break; | ||
|
|
||
| case ASYNC_INITIAL_INSTALL_MODE_ONLY_REQUIRED_RESOURCES: { | ||
| // TODO: Add AsyncPCK contents to the cache. |
There was a problem hiding this comment.
I assume this TODO is blocking for the PR?
There was a problem hiding this comment.
I'll check if it's still needed.
2a795bf to
bf811fd
Compare
Also:
- Add bindings to `ResourceLoader::{load,load_threaded_*} methods`.
- Add Web platform support to export a project using AsyncPCK.
- Add Web platform backend code to support AsyncPCK `OS` calls.
bf811fd to
cce6bd2
Compare
Supersedes #114690
Changes from the previous PR
This PR is essentially #114690 rebased to
master, but with a few changes following the TLC comments.AsyncPckInstallernode which was an developer utility to speed up integration of Async in their project.OS::async_pck_*methods from the public API.ResourceLoader.load_threaded_*API.The idea behind putting the AsyncPCK logic behind
ResourceLoader.load_threaded_*was twofold:Important caveats when using
ResourceLoader.load_threaded_*instead of a dedicated AsyncPCK APIIn my opinion, I think that the fact that
load()cannot stop the engine on the Web1 until the missing files are downloaded and installed really complicates everything.ResourceLoader.load_threaded_*have now multiple caveats attached to them. (see screenshots below)ResourceLoader.load_threaded_*API, effectively banning the simplerload()method.ResourceLoader.load_threaded_get_status()progress value. (IMHO, the fact that Godot doesn't display that data (or offer devs to choose to) on the first load is a bug, not a feature.)Docs for
ResourceLoader.load_threaded_*Yellow boxes are for emphasis.
ResourceLoadermethodResourceLoader.load_threaded_get()ResourceLoader.load_threaded_get_status()ResourceLoader.load_threaded_request()Footnotes
It's not possible without building the WASM file with Emscripten's Asyncify API (which really bumps up the size of the WASM output, which kinda defeats the purpose of the PR to load games faster on Web). ↩