Skip to content
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

Public AssetListLoader & examples update #4149

Merged
merged 15 commits into from
Apr 6, 2022
Merged

Conversation

ellthompson
Copy link
Contributor

@ellthompson ellthompson commented Mar 22, 2022

This PR brings back the AssetListLoader class which allows an array of assets to be loaded before the 'ready' callback is called. This class is made public and includes unit tests.

The examples browser now makes use of the AssetListLoader for asset loading in all examples. The examples browser therefore no longer needs to hoist the application instantiation in each example in order to preload example assets in.

New Public API:

// Create a new AssetListLoader using a list of assets to load and the asset registry used to load and manage them.
pc.AssetListLoader.constructor(assetList: pc.Asset[], assetRegistry: pc.AssetRegistry): void

// Destroys the AssetListLoader.
pc.AssetListLoader.destroy(): void

// Start loading asset list, call done() when all assets have loaded or failed to load.
pc.AssetListLoader.load(done: Function, scope: ?object): void

// Sets a callback which will be called when all assets in the list have been loaded.
pc.AssetListLoader.ready(done: Function, scope: ?object): void

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

package.json Outdated Show resolved Hide resolved
src/asset/asset.js Outdated Show resolved Hide resolved
@willeastcott
Copy link
Contributor

Looks good to me, in general. I've left a bunch of comments. However, should we take a moment to review the AssetListLoader API? Do we like the naming? Is the public interface properly established? Do the function signatures make sense (e.g. the constructor taking the asset registry as the second param instead of the first)? Should this functionality just be a function on the AssetRegistry directly instead?

@Maksims
Copy link
Collaborator

Maksims commented Mar 24, 2022

Would it be possible to write a new API in similar format in the first post of this PR?

@mvaligursky mvaligursky added the release: next minor Ticket marked for the next minor release label Mar 24, 2022
@ellthompson
Copy link
Contributor Author

ellthompson commented Mar 24, 2022

Would it be possible to write a new API in similar format in the first post of this PR?

Sure, I've added this in.

@ellthompson ellthompson marked this pull request as ready for review March 24, 2022 16:28
@ellthompson ellthompson requested a review from a team March 24, 2022 16:32
@mvaligursky
Copy link
Contributor

AssetListLoader.load(done: Function, scope: ?object): void
AssetListLoader.ready(done: Function, scope: ?object): void

I just wonder if the ready function is needed? It does not seem to expose anything additional to the load function. Is this being used by the examples?

@ellthompson
Copy link
Contributor Author

AssetListLoader.load(done: Function, scope: ?object): void
AssetListLoader.ready(done: Function, scope: ?object): void

I just wonder if the ready function is needed? It does not seem to expose anything additional to the load function. Is this being used by the examples?

It's not used by the examples but it allows other parts of an application to subscribe to the loading of these assets without triggering the load itself.

@ellthompson
Copy link
Contributor Author

I'm not sure it works like that .. all browsers apart from IE11 implement the function, and that is accessible in ES5 as well .. we don't need a polyfill for ES5, but for IE11. Perhaps check ES5 version of the engine built - i likely just calls this function directly.

Yeah you're right, i've included this in the list of example browser polyfills.

@ellthompson ellthompson requested a review from mvaligursky April 6, 2022 15:52
Copy link
Contributor

@mvaligursky mvaligursky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving, nicely done

@ellthompson ellthompson merged commit d50e09d into main Apr 6, 2022
@ellthompson ellthompson deleted the asset-list-loader branch April 6, 2022 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: assets area: examples release: next minor Ticket marked for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants