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.

Sign up for GitHub

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

Jump to bottom

Returning conversion warnings in 3DMLoader #21639

Merged
merged 30 commits into from
May 18, 2021
Merged

Returning conversion warnings in 3DMLoader #21639

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
158a875
extract name and value properties from objects
fraguada Feb 11, 2021
ec3ff76
Extract object userStrings
fraguada Feb 15, 2021
89da0c7
Merge branch 'dev' of https://github.com/mrdoob/three.js into wip/3DM…
fraguada Feb 15, 2021
78afc99
Merge branch 'wip/3DMLoader' of https://github.com/mcneel/three.js in…
fraguada Apr 15, 2021
c810381
3dmLoader: refactor es6
fraguada Apr 15, 2021
4176142
Remove unnecessary imports.
fraguada Apr 15, 2021
1d97c81
Store conversion warnings in returned object
karimi Apr 13, 2021
e21f211
Pass worker erros to onError callback
karimi Apr 13, 2021
da176ab
Pass worker erros to onError callback
karimi Apr 13, 2021
6c02601
Merge branch 'dev' into 3dmloaderwarnings
karimi Apr 26, 2021
ea34dbf
adding some error catching related to PBR mats
fraguada Apr 30, 2021
103675f
Merge branch 'dev' of https://github.com/mrdoob/three.js into wip/3DM…
fraguada Apr 30, 2021
7731833
fix bug in texturetype checking
fraguada Apr 30, 2021
0652c61
Post warnings to worker
karimi May 3, 2021
b1ecb5e
Merge https://github.com/mrdoob/three.js into 3dmloaderwarnings
karimi May 3, 2021
27b8f45
Merge branch 'dev' of https://github.com/mrdoob/three.js into wip/3DM…
fraguada May 5, 2021
92a0c62
adding taskId in worker
fraguada May 7, 2021
6a35bd8
Merge branch 'wip/3DMLoader' of https://github.com/mcneel/three.js in…
fraguada May 7, 2021
0f2ac4a
Merge branch 'dev' of https://github.com/mrdoob/three.js into 3dmload…
fraguada May 7, 2021
cb633a0
cleanup taskID usage
fraguada May 7, 2021
e3a7f58
added class level warnings
fraguada May 7, 2021
3cdfabe
printing warnings
fraguada May 7, 2021
e9d7654
fixed bug in .catch
fraguada May 7, 2021
0c60f5f
Fix undefined this.warnings
karimi May 7, 2021
5d06888
Add onWarning callback
karimi May 7, 2021
3ca34c0
Rewriting onMessage as arrow function
karimi May 7, 2021
bd86376
rework warning messages, lightstyle switch
fraguada May 11, 2021
59dd4bd
Remove onWarning callback, add warnings to userData
karimi May 12, 2021
7f8bcc4
Cleanup comments, etc.
fraguada May 17, 2021
a7806e6
reverted change in example
fraguada May 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 37 additions & 22 deletions examples/jsm/loaders/3DMLoader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ class Rhino3dmLoader extends Loader {

this.decodeObjects( buffer, url )
.then( onLoad )
.catch( onError );
.catch( e => onError( e ) );

}, onProgress, onError );

Expand All @@ -106,6 +106,7 @@ class Rhino3dmLoader extends Loader {

let worker;
let taskID;
let warnings =[];

const taskCost = buffer.byteLength;

Expand All @@ -117,16 +118,24 @@ class Rhino3dmLoader extends Loader {

return new Promise( ( resolve, reject ) => {

worker._callbacks[ taskID ] = { resolve, reject };
worker._callbacks[ taskID ] = { resolve, reject, warn: warning =>warnings.push(warning) };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a function to collate warnings thrown by workers.


worker.postMessage( { type: 'decode', id: taskID, buffer }, [ buffer ] );

//this.debug();
// //this.debug();

} );

} )
.then( ( message ) => this._createGeometry( message.data ) );
.then( ( message ) => {
const geometry = this._createGeometry( message.data, taskID );
geometry.warnings = warnings;
return geometry;
}).catch( e => {

throw e;

} );

// Remove task from the task list.
// Note: replaced '.finally()' with '.catch().then()' block - iOS 11 support (#19416)
Expand Down Expand Up @@ -160,7 +169,7 @@ class Rhino3dmLoader extends Loader {

this.decodeObjects( data, '' )
.then( onLoad )
.catch( onError );
.catch( e => onError( e ) );

}

Expand Down Expand Up @@ -281,7 +290,7 @@ class Rhino3dmLoader extends Loader {

}

_createGeometry( data ) {
_createGeometry( data, taskID ) {

// console.log(data);

Expand Down Expand Up @@ -328,12 +337,12 @@ class Rhino3dmLoader extends Loader {
const rMaterial = materials[ attributes.materialIndex ];
let material = this._createMaterial( rMaterial );
material = this._compareMaterials( material );
_object = this._createObject( obj, material );
_object = this._createObject( obj, material, taskID );

} else {

const material = this._createMaterial( );
_object = this._createObject( obj, material );
_object = this._createObject( obj, material, taskID );

}

Expand Down Expand Up @@ -422,7 +431,7 @@ class Rhino3dmLoader extends Loader {

}

_createObject( obj, mat ) {
_createObject( obj, mat, taskID ) {

const loader = new BufferGeometryLoader();

Expand Down Expand Up @@ -615,7 +624,7 @@ class Rhino3dmLoader extends Loader {

} else if ( geometry.isLinearLight ) {

console.warn( 'THREE.3DMLoader: No conversion exists for linear lights.' );
self.postMessage( { type: 'warning', id: taskID, message: 'THREE.3DMLoader: No conversion exists for linear lights.' } );
Copy link
Contributor

Choose a reason for hiding this comment

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

@karimi here we are no longer in the worker. What do you want to do with these warnings? I'm don't think posting messages is the way to go here.


return;

Expand Down Expand Up @@ -708,6 +717,9 @@ class Rhino3dmLoader extends Loader {
const message = e.data;

switch ( message.type ) {
case 'warning':
worker._callbacks[ message.id ].warn( message );
Copy link
Contributor Author

@karimi karimi May 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@fraguada We need a id to figure out which task this warning belongs to. I'm using the taskID corresponding to the file to collate the warnings.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure we should add a .warn to this callback, since the promises api only defines resolve and reject. I think this is the place we should be collecting the warning, or printing them to the console, or doing something. The worker should need to be bothered by a warning unless I am confused.

break;

case 'decode':
worker._callbacks[ message.id ].resolve( message );
Expand Down Expand Up @@ -809,9 +821,16 @@ function Rhino3dmWorker() {
const buffer = message.buffer;
libraryPending.then( () => {

const data = decodeObjects( rhino, buffer );
try {

const data = decodeObjects( rhino, buffer, message.id );
karimi marked this conversation as resolved.
Show resolved Hide resolved
self.postMessage( { type: 'decode', id: message.id, data } );

} catch ( error ) {

self.postMessage( { type: 'decode', id: message.id, data } );
self.postMessage( { type: 'error', id: message.id, error } );

}

} );

Expand All @@ -821,7 +840,7 @@ function Rhino3dmWorker() {

};

function decodeObjects( rhino, buffer ) {
function decodeObjects( rhino, buffer, taskID ) {
karimi marked this conversation as resolved.
Show resolved Hide resolved

const arr = new Uint8Array( buffer );
const doc = rhino.File3dm.fromByteArray( arr );
Expand All @@ -842,7 +861,7 @@ function Rhino3dmWorker() {

const _object = objs.get( i );

const object = extractObjectData( _object, doc );
const object = extractObjectData( _object, doc, taskID );
karimi marked this conversation as resolved.
Show resolved Hide resolved

_object.delete();

Expand Down Expand Up @@ -926,10 +945,8 @@ function Rhino3dmWorker() {
texture.image = 'data:image/png;base64,' + image;

} else {

console.warn( `THREE.3DMLoader: Image for ${textureType} texture not embedded in file.` );
self.postMessage( { type: 'warning', id: taskID, message:`THREE.3DMLoader: Image for ${textureType} texture not embedded in file.` } );
texture.image = null;

}

textures.push( texture );
Expand Down Expand Up @@ -1064,7 +1081,7 @@ function Rhino3dmWorker() {

}

function extractObjectData( object, doc ) {
function extractObjectData( object, doc, taskID ) {
karimi marked this conversation as resolved.
Show resolved Hide resolved

const _geometry = object.geometry();
const _attributes = object.attributes();
Expand Down Expand Up @@ -1225,7 +1242,7 @@ function Rhino3dmWorker() {
*/

default:
console.warn( `THREE.3DMLoader: TODO: Implement ${objectType.constructor.name}` );
self.postMessage( { type: 'warning', id: taskID, message:`THREE.3DMLoader: TODO: Implement ${objectType.constructor.name}` } );
break;

}
Expand Down Expand Up @@ -1261,9 +1278,7 @@ function Rhino3dmWorker() {
return { geometry, attributes, objectType };

} else {

console.warn( `THREE.3DMLoader: ${objectType.constructor.name} has no associated mesh geometry.` );

self.postMessage( { type: 'warning', id: taskID, message:`THREE.3DMLoader: ${objectType.constructor.name} has no associated mesh geometry.` } );
}

}
Expand Down
2 changes: 1 addition & 1 deletion examples/webgl_loader_3dm.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@
// hide spinner
document.getElementById( 'loader' ).style.display = 'none';

} );
}, progress =>progress, error => ( console.log( error ) ) );
Copy link
Owner

Choose a reason for hiding this comment

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

What does progress =>progress do?

Copy link
Contributor

Choose a reason for hiding this comment

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

Cleaned that up.


controls = new OrbitControls( camera, renderer.domElement );

Expand Down