playcanvas
Remove implicit loading of assets, when adding to asset registry
Description
This PR removes the implicit loading of assets with preload=true, when they are added to the asset registry.
Fixes #3107
I hope this gets some traction here, this is currently blocking our custom loading solution when using multiple scenes https://forum.playcanvas.com/t/assetregistry-not-available-in-custom-loading-script-v2/40918
Checklist
- [x] I have read the contributing guidelines
- [x] My code follows the project's coding standards
- [x] This PR focuses on a single change
Can this be closed now due to https://github.com/playcanvas/engine/pull/8177 ?
This PR is independent from #8177.
This PR moves loading of preloaded assets from AssetRegistry.add to AppBase.preload.
The other PR implements AssetListLoader in AppBase.preload to load preloaded assets instead of using the current custom solution.
This looks like a breaking change, as it will change the logic of other systems? Has this been tested and verified with various cases where people might expect for assets to be loaded. And if the behavior will change with this PR - then it cannot be pushed as is. If that is the case, then there should be some flag, that by default preserves current behavior, but allows also to disable auto-loading if some flag is switched off.
Agreed that this is indeed changing behaviour.
If a asset with preload set to true is added at runtime, you now additionally need to call app.assets.load(asset)
Previously app.assets.add(asset) would have been enough.
However the current behaviour is not in line with the docs for Asset.preload:
Sets whether to preload an asset. If true, the asset will be loaded during the preload phase of application set up.
So based on that, we cannot risk breaking people's app logic, as behavior will change. And it should be "opt-in" feature - via the flag.
I'd still advocate to change this.
Is it breaking behaviour if the behaviour based on the docs was never intended to work this way? I'd say this is a bug and even if someone relies on this behaviour they didn't implement it correctly in the first place.
I think the impact on the user base will be minimal.
We can just as well say the issue is in the docs and we fix the docs. Many things are complicated to change due to this, and we always try to find some solution to keep existing users happy, at least in common cases.
If we dont want to change this behaviour it should definitely be documented.
Unfortunetly this wont solve my problem of customizing loading behaviour, before app.assets.add is called, when exporting from the editor.
However this can also be solved by adding an event in AppBase.init. Are you open to this?
However this can also be solved by adding an event in AppBase.init. Are you open to this?
Totally, a useful generic event at the right place is a good option for many use cases.