-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
Returning conversion warnings in 3DMLoader #21639
Conversation
/ping @fraguada |
@mrdoob @Rada8732 @Mugen87 @karimi I'd like a bit of time to review this. @karimi and I have discussed the use case, which is valid, but I'd like to see if there are any other options to bubble up errors from workers. I haven't done the research yet, but I'm guessing this can be done through messaging. Any suggestions are welcome. |
@karimi I believe the way we'd want to do this is to actually use the worker messaging like is done here: https://github.com/mrdoob/three.js/blob/dev/examples/jsm/loaders/3DMLoader.js#L811 We could extend this to Does this sound reasonable? |
@fraguada are you suggesting we add a
I'm not sure how that can work since warnings can be thrown at individual object level and tasks are per buffer. Of course that can change, but it's a bigger change. |
…to wip/3DMLoader # Conflicts: # examples/jsm/loaders/3DMLoader.js
38e1a96
to
da176ab
Compare
@Mugen87 Just rebased and pushed. |
@fraguada did you have a chance to review this? |
…Loader # Conflicts: # examples/jsm/loaders/3DMLoader.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karimi I've written some comments along with your proposed changes. I believe we can still take advantage of the worker self.postMessage( { type: 'warning', data } );
pattern by adding a warning case when setting up the worker (around line 706). When the worker finishes, you'd have a collection of warnings that can be checked along with the resulting objects. What do you think?
examples/jsm/loaders/3DMLoader.js
Outdated
@@ -1226,6 +1241,7 @@ function Rhino3dmWorker() { | |||
|
|||
default: | |||
console.warn( `THREE.3DMLoader: TODO: Implement ${objectType.constructor.name}` ); | |||
onWarning( `THREE.3DMLoader: TODO: Implement ${objectType.constructor.name}` ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same strategy as suggested above
examples/jsm/loaders/3DMLoader.js
Outdated
@@ -714,6 +717,9 @@ class Rhino3dmLoader extends Loader { | |||
const message = e.data; | |||
|
|||
switch ( message.type ) { | |||
case 'warning': | |||
worker._callbacks[ message.id ].warn( message ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
examples/jsm/loaders/3DMLoader.js
Outdated
@@ -117,7 +118,7 @@ class Rhino3dmLoader extends Loader { | |||
|
|||
return new Promise( ( resolve, reject ) => { | |||
|
|||
worker._callbacks[ taskID ] = { resolve, reject }; | |||
worker._callbacks[ taskID ] = { resolve, reject, warn: warning =>warnings.push(warning) }; |
There was a problem hiding this comment.
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.
@fraguada Thanks for your suggestions, I think posting the warnings to the workers makes sense. I was able to do that but needed an |
@karimi the warnings should only occur in a worker that has a So for example: /* WEB WORKER */
function Rhino3dmWorker() {
let libraryPending;
let libraryConfig;
let rhino;
let taskId; //new and then case 'decode':
taskId = message.id; // new
const buffer = message.buffer;
libraryPending.then( () => {
const data = decodeObjects( rhino, buffer );
self.postMessage( { type: 'decode', id: message.id, data } );
} );
break; |
examples/jsm/loaders/3DMLoader.js
Outdated
@@ -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.' } ); |
There was a problem hiding this comment.
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.
@karimi I made some changes and updates. Thanks for giving me push access to your fork, it makes this PR process better IMO. I think with these changes we cover a general way to communicate warnings from the worker. In the main code we need to decide how you want to use these warnings. For now, I created a class level |
@fraguada I turned this into a draft PR since there are still issues that need to be resolved. I pushed a commit to fix the issue with undefined |
@fraguada I have a few ideas on how to return these load( url, onLoad, onProgress, onError, onWarning ) {
.
.
.
this.decodeObjects( buffer, url )
.then( result =>{onLoad(result); this.warnings.length>0 && onWarning(this.warnings)} )
.catch( e => onError( e ) );
}, onProgress, onError );
} |
Regarding the contents of the warnings I think they're already useful, but it would be definitely an improvement if we could return an object with warning |
@karimi I've reworked the warning message style, adding a guid property when the warning is related to an object. Warning types are:
I think we should go with your suggestion to add warnings to the objects (when possible) and not add an onWarning callback. I suggest objects get a warning property in the object userData with the warnings associated with the object guid. This won't be relevant for things like missing textures. If that is important, we can handle it in another PR. |
Thank @fraguada , I think the warning object is more informative now. |
examples/webgl_loader_3dm.html
Outdated
@@ -76,7 +76,7 @@ | |||
// hide spinner | |||
document.getElementById( 'loader' ).style.display = 'none'; | |||
|
|||
} ); | |||
}, progress =>progress, error => ( console.log( error ) ) ); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaned that up.
Thanks! |
Fixed #21638
Description
3DMLoader
parse
andload
methods can partially fail to convert some entities of the original file for various reasons including missing render meshes. This PR captures returns warnings of this nature.