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

Added onLoadStart & onLoadComplete callbacks #440

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pizza3
Copy link
Contributor

@pizza3 pizza3 commented Jan 6, 2024

This pr adds the callback onLoadStart & onLoadComplete for all loading starts and ends.
#389

How to test:
I used mars.js example for the test and assigned onLoadStart & onLoadComplete callbacks to the tiles.

Copy link
Contributor

@gkjohnson gkjohnson left a comment

Choose a reason for hiding this comment

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

Awesome, thank you! I've added a couple comments. A few other things to that should be added:

  • add initialization for onLoadStart and onLoadComplete to the constructor
  • update d.ts file
  • update README

if ( this.onLoadComplete ) {

this.onLoadComplete( );
this.loading = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

this.loading should be set to false regardless of whether onLoadComplete is set.

@@ -675,6 +676,29 @@ export class TilesRenderer extends TilesRendererBase {
cached.scene = scene;
cached.metadata = metadata;

if ( ! this.loading && this.parseQueue.items.length > 0 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should check this.parseQueue.items.length > 0 || this.downloadQueue.items.length > 0


}

if ( this.loading && this.parseQueue.items.length === 0 && this.downloadQueue.items.length === this.parseQueue.items.length ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not check if this.downloadQueue.items.length === 0?

Comment on lines +679 to +688
if ( ! this.loading && this.parseQueue.items.length > 0 ) {

if ( this.onLoadStart ) {

this.onLoadStart( );

}
this.loading = true;

}
Copy link
Contributor

Choose a reason for hiding this comment

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

It occurs to me that this will not trigger if we're in a case where only one tile has downloaded - because this condition will be checked only once a file has been fully parsed. Perhaps this "onStart" condition should go at the end of the update function, instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants