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

Loader Improvements #4248

Closed
gregtatum opened this issue Dec 27, 2013 · 17 comments
Closed

Loader Improvements #4248

gregtatum opened this issue Dec 27, 2013 · 17 comments

Comments

@gregtatum
Copy link
Contributor

I'm taking a crack at improving some of the loaders, making them a little more consistent in their usage, and then writing out the docs. I'll post anything I need feedback on here.

First thing:

THREE.Loader has an addStatusElement method, which creates a DOM element that says "Loading ...". It doesn't add it to the DOM, update it, or do anything with it. I was thinking about removing that method as it doesn't appear to do anything, and is not used in the examples.

It seems that if there is any DOM loading interface it should go in THREE.LoadingManager. In fact maybe a visual interface should be a separate class that is only in the examples.

So my proposal is to remove that functionality, and if it gets put back in, put it into the examples. Thoughts?

@gero3
Copy link
Contributor

gero3 commented Dec 27, 2013

It gets used in the examples.
https://github.com/mrdoob/three.js/search?q=statusDomElement
This is the domElement that gets added when in the constructor of Loader, showstatus has been set.

Altough I agree it needs some improvements. It would be best if the manager had a simple default loading DOM.
That is easily overwritten in the examples.

@mrdoob
Copy link
Owner

mrdoob commented Dec 28, 2013

So my proposal is to remove that functionality, and if it gets put back in, put it into the examples. Thoughts?

Sounds good to me 👍

@gregtatum
Copy link
Contributor Author

How crazy would it be to look at using promises for loaders and the loading manager? I have a bunch of code going down this path and I'm wondering if it's worth pursuing. I'm not sure about dependencies and getting something that could be a self-contained promise implementation for use inside the three.js codebase. Promises seem to make things work out nice at the end of it. The end user would then have an onLoad callback, and a promise that could be used to interact with.

@mrdoob
Copy link
Owner

mrdoob commented Apr 12, 2014

I've yet to understand promises. Do you know of a page doing a good explanation. At the moment, I don't understand the difference between callbacks and promises.

@gregtatum
Copy link
Contributor Author

http://www.html5rocks.com/en/tutorials/es6/promises/
http://promises-aplus.github.io/promises-spec/
http://blog.parse.com/2013/01/29/whats-so-great-about-javascript-promises/

Essentially you can pass promises around, and you don't really have to care specifically when they are resolved. So you can load external shaders, images, models, etc, then use a promise to determine when to start the execution of your game logic or whatever else, only when the promised results have been resolved. It also helps get rid of callback hell and makes code easier to read since callbacks don't have to be crazily nested. It also makes error handling happen in a much more logical order.

A bunch of examples using Q: https://github.com/kriskowal/q/wiki/Examples-Gallery
Here's a simple chaining example using Q: https://gist.github.com/jeffcogswell/8257755

@kumavis
Copy link
Contributor

kumavis commented Apr 13, 2014

Promises vs Callbacks is a pretty opinionated topic.

Personally I feel like callbacks are the simplest implementation, and if you want the Loader to provide a promise, you can convert the operation yourself using something like q-wrap

This is how q-wrap does its magic by the way.

@gregtatum
Copy link
Contributor Author

Yeah, I think you're right about it being too opinionated a topic. I wrote some code going down this path, and left it for awhile. I'll just use the q-wrap in my own stuff. That looks like a handy tool. It seems like it would be trivial to implement.

@repsac
Copy link
Contributor

repsac commented Apr 13, 2014

TIL: Promises.

Thanks for sharing at least. I see the appeal but because of the work I do I have to think about backwards compatibility and that companies I deal with, or their clients, may not have the most updated browsers (in fact they are normally at least a year behind) so I would be concerned about depending on a native implementation of Promises being present.

@SET001
Copy link
Contributor

SET001 commented Jul 27, 2015

  • 100500 for using promises instead of callbacks

@benaadams
Copy link
Contributor

Isn't too hard to switch to promises in the calling code, using the existing callback system.

atm we use https://github.com/taylorhakes/promise-polyfill as a promise polyfill as its tiny

Wrap our texture loads in promises, with caching and cpu-side dumping post gpu load something like this:

var texturePromises = {};

THREE.ImageUtils.crossOrigin = "anonymous";

function SetTextureAsync( path, wrapping, uniform ) {
    var promise;
    var hashPath = path + '|' + wrapping;
    if ( texturePromises[hashPath] ) {
        promise = texturePromises[hashPath];
    } else {
        promise = LoadTextureAsync( path );
        texturePromises[hashPath] = promise;
    }
    return promise.then( updateLoadStatus )
            .then( function ( texture ) {
                texture.wrapS = texture.wrapT = wrapping;
                uniform.value = texture;
            } );
}

function SetCompressedTextureAsync( path, wrapping, uniform ) {
    var promise;
    var hashPath = path + '|' + wrapping;
    if ( texturePromises[hashPath] ) {
        promise = texturePromises[hashPath];
    } else {
        promise = LoadCompressedTextureAsync( path );
        texturePromises[hashPath] = promise;
    }
    return promise.then( updateLoadStatus )
            .then( function ( texture ) {
            texture.wrapS = texture.wrapT = wrapping;
            uniform.value = texture;
    } );
}

var LoadTextureAsync = ( function () {
    function ReleaseTexture( texture ) {
        texture.image = null;
        if ( texture.mipmaps ) {
            texture.mipmaps = null;
        }
        texture.onUpdate = undefined;
    }
    return function LoadTextureAsync( texturePath ) {
        return new Promise( function ( resolve, reject ) {
            var texture = THREE.ImageUtils.loadTexture( pathTextures + texturePath, undefined, resolve, reject );
            console.log( "* Loading " + texturePath );
            texture.onUpdate = ReleaseTexture;
        } );
    }
} )();

var LoadCompressedTextureAsync = ( function () {
    var ddsLoader = new THREE.DDSLoader();
    function ReleaseTexture( texture ) {
        if ( texture.mipmaps ) {
            texture.image = null;
            texture.mipmaps = null;
        }
        texture.onUpdate = undefined;
    }
    return function LoadCompressedTextureAsync( texturePath ) {
        return new Promise( function ( resolve, reject ) {
            var texture = ddsLoader.load( pathTextures + texturePath + '.dds' + ext, resolve, undefined, reject );
            console.log( "* Loading " + texturePath + '.dds' + ext );
            texture.onUpdate = ReleaseTexture;
        } );
    }
} )();

Have an item that holds all the start up promises:

var startupResources = [];

Create a bunch of texture uniforms and load the promises into the startupResources

var textures = {};

function downloadTextures() {
    textures["tileTexture"] = { type: "t", value: null };
    startupResources.push( SetCompressedTextureAsync( "terrain_r", 
        THREE.ClampToEdgeWrapping, textures["tileTexture"] ) );
    ...
}

Have an animate function that switches to render when the loadStage is Running

render has its own call to requestAnimationFrame so essentially this function is dumped when everything is loaded.

function animate() {
    if ( loadStage === loadStages.Running ) {
        render();
    } else if ( loadStage === loadStages.Loading ) {
        tickLoader();
        requestAnimationFrame( animate );
    }
}

In the init it kicks off all the downloads which add them to startupResources as per downloadTextures above.

We then use Promise.all( startupResources ) to wait until they all complete before switching loadStage = loadStages.Running which triggers the animate function to move to rendering rather than an animated loading screen

World.init = function init() {

    if ( setupRenderer() !== true ) return false;

    downloadExtraData();
    downloadShaders();
    downloadTextures();

    animate();

    startWorker();

    Promise.all( startupResources )
        .then( function () {
            needsResize = true;
            startupResources = texturePromises = undefined;
            document.getElementById( "loadingText" ).style.display = "none";
            document.getElementById( "progressBar" ).style.display = "none";
            document.getElementById( "logoLoading" ).classList.add( "disappear" );
            document.getElementById("topbar").classList.remove( "off" );
            setTimeout( function () {
                document.getElementById( "logoLoading" ).style.display = "none";
            }, 3000 );
            console.info( "All loaded" );

            SetUpModels();
            SetupUIEvents();

            loadStage = loadStages.Running;
        } )
        .catch( function ( e ) {
            console.error( "Error", e );
            loadStage = loadStages.Errored;
        } );

    return true;
}

@lukehorvat
Copy link
Contributor

lukehorvat commented Oct 1, 2017

If promises are too opinionated, perhaps loaders could at least use the common Node.js callback style i.e. function(error, value) { }? Then three.js wouldn't need to use promises, but folks could easily promisify loaders if they wanted to.

Callback example:

var loader = new THREE.ImageLoader();

loader.load('texture.png', function(error, image) {
  if (error) {
    // Do something with the error.
  } else {
    // Do something with the image.
  }
});

Promisified example:

var util = require('util');
var loader = new THREE.ImageLoader();

util.promisify(loader.load)('texture.png')
.then(function(image) {
  // Do something with the image.
})
.catch(function(error) {
  // Do something with the error.
});

@mrdoob
Copy link
Owner

mrdoob commented Oct 2, 2017

If promises are too opinionated, perhaps loaders could at least use the common Node.js callback style i.e. function(error, value) { }?

Is there a spec for that?

@lukehorvat
Copy link
Contributor

The closest I've found to a spec is this. And it is explained in the Node.js documentation here.

It's just a Node.js convention, and pretty much every library that provides promisification (including ones that aren't tied to Node.js and work in the browser) follows this convention.

You can read more about it here.

@looeee
Copy link
Collaborator

looeee commented Oct 3, 2017

I've been promisifying the current loaders for quite a while now without issues. I don't think that they need to be refactored for this.

@satori99
Copy link
Contributor

satori99 commented Oct 3, 2017

Hi all, One thing that springs to my mind regarding Promises, is that they allow for ES6 async / await style coding, which can help to simplify highly asynchronous code in many cases.

@donmccurdy
Copy link
Collaborator

THREE.Loader no longer has any DOM/status management code, so I think this issue can be closed? If we want to do anything different with promises vs callbacks that could be opened in a new issue.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 27, 2017

If we want to do anything different with promises vs callbacks that could be opened in a new issue.

Yeah, sounds good. Let's close this one.

@Mugen87 Mugen87 closed this as completed Dec 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests