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

datasources/base.js ImageLoader.load - Error callback broken #43

Open
bitroost opened this issue Jul 10, 2022 · 0 comments
Open

datasources/base.js ImageLoader.load - Error callback broken #43

bitroost opened this issue Jul 10, 2022 · 0 comments

Comments

@bitroost
Copy link

The imageLoader onProgress-callback is missing from the ImageLoader.load-call so the error callback never triggers in case of a tile that is missing.
From the Three.js ImageLoader documentation:

// load a image resource
// instantiate a loader
const loader = new THREE.ImageLoader();

loader.load(
	// resource URL
	'textures/skyboxsun25degtest.png',

	// onLoad callback
	function ( image ) {
		// use the image, e.g. draw part of it on a canvas
		const canvas = document.createElement( 'canvas' );
		const context = canvas.getContext( '2d' );
		context.drawImage( image, 100, 100 );
	},

	// onProgress callback currently not supported
	undefined,

	// onError callback
	function () {
		console.error( 'An error happened.' );
	}
);

To Reproduce
The issue becomes noticeable when using a custom datasource with a lot of requests returning 204 for 'Tile does not exist' using Tileserver-GL (maptiler/tileserver-gl#278).
What happens is that the request errors do not get deleted from this.fetching, pile up there and eventually trigger throttling which, in turn, after 32 errors, blocks the request of any new tile.

datasources/base.js #95-100

    // Throttle downloads
    let downloadIndices = Object.values( this.fetching );
    if ( downloadIndices.length > 32 ) {
      log( 'throttling...' );
      return;
    }

Expected behavior
With the error callback working, the errors get deleted from this.fetching and do not trigger throttling, but in case of 'missing tiles' will try again and retrigger the error.

Proposed fix
I think errors should be stored in an object similar to this.fetching

    this.fetching = {};
    this.failed = {};

populated on error and then checked against at the beginning of fetchIfNeeded():

  fetchIfNeeded( quadkey ) {
    if ( this.lookup[ quadkey ] !== undefined ||
       this.fetching[ quadkey ] !== undefined ||
       this.failed[ quadkey ] ) {
      // Have data, download in progress, or tried and failed: skip
      return;
    }

    // Throttle downloads
    let downloadIndices = Object.values( this.fetching );
    if ( downloadIndices.length > 32 ) {
      log( 'throttling...' );
      return;
    }

    let newIndex = this.findNewIndex( quadkey );

    // Mark as download in progress
    this.fetching[ quadkey ] = newIndex;

    // Actually fetch data
    let url = this.urlForTile( ...tilebelt.quadkeyToTile( quadkey ) );
    ImageLoader.load( url, ( image ) => {
      // Image loaded OK
      this.imgCache[ quadkey ] = image;
      insertIntoTextureArray( this.textureArray, newIndex, image );

      // Remove download and mark image location in lookup
      delete this.fetching[ quadkey ];
      this.lookup[ quadkey ] = newIndex;

      // TODO remove, just for demo page
      if ( this.useFloat ) {
        let el = document.getElementById( 'elevation-tile-count' );
        if ( el ) {
          let n = parseInt( el.innerHTML );
          if ( isNaN( n ) ) { n = 0 }

          el.innerHTML = ++n;
        }
      }

      this.updateIndirectionTexture();
      this.notifyUpdate();
    },
    undefined,
    ( err ) => {
      if ( err.name == 'InvalidStateError' ) {
        let newIndex = this.findNewIndex( quadkey );
        this.failed[ quadkey ] = newIndex;
        log( 'InvalidStateError / HTTP 204', quadkey );
      } else {
        console.error( 'Failed to get image', quadkey );
      }

      delete this.fetching[ quadkey ];
    } );
  }

Http 204 produces 'InvalidStateError' which should not trigger a console.error but everything else still does.

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

No branches or pull requests

1 participant