search

mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
101.71k stars 35.3k forks source link

VRMLLoader does no error forwarding calling the parser #19404

Closed stewe closed 4 years ago

stewe commented 4 years ago
Description of the problem

When an error occurs while parsing the loaded VRML file, the VRMLLoader does not handle it.

        load: function ( url, onLoad, onProgress, onError ) {

            var scope = this;

            var path = ( scope.path === '' ) ? LoaderUtils.extractUrlBase( url ) : scope.path;

            var loader = new FileLoader( this.manager );
            loader.setPath( scope.path );
            loader.load( url, function ( text ) {

                onLoad( scope.parse( text, path ) );

            }, onProgress, onError );

        },

It should call onError instead.

        load: function ( url, onLoad, onProgress, onError ) {

            var scope = this;

            var path = ( scope.path === '' ) ? LoaderUtils.extractUrlBase( url ) : scope.path;

            var loader = new FileLoader( this.manager );
            loader.setPath( scope.path );
            loader.load( url, function ( text ) {

              try {
                const parsed = scope.parse( text, path )
                  onLoad(parsed);

                         } catch (e) {
                            onError(e)
                       }

            }, onProgress, onError );

        },
Three.js version
Browser
OS
Mugen87 commented 4 years ago

It seems VRMLLoader is not the only loader which does not use try/catch around the invocation of parse(). It might be worth to check all loaders and change the code according to GLTFLoader.

mrdoob commented 4 years ago

I think loaders should use try/catch. It's annoying when it bubbles up to the Editor's Loader.js.

Mugen87 commented 4 years ago

The code should actually look like so since onError() is no mandatory callback function. It's also useful to tell the loading manager that an error occured.

try {

    onLoad( scope.parse( text, path ) );

} catch ( e ) {

    if ( onError ) {

        onError( e );

    } else {

        console.error( e );

    }

    scope.manager.itemError( url );

}
mrdoob commented 4 years ago

Looks good to me!

Mugen87 commented 4 years ago

Okay, I've added today a PR with the fix.