-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Remove implicit loading of assets, when adding to asset registry #8176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove implicit loading of assets, when adding to asset registry #8176
Conversation
|
Can this be closed now due to #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? |
|
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 However the current behaviour is not in line with the docs for Asset.preload:
|
|
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. |
|
If we dont want to change this behaviour it should definitely be documented. Unfortunetly this wont solve my problem of customizing loading behaviour, before |
Totally, a useful generic event at the right place is a good option for many use cases. |
|
Closing in favour of #8189 |
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