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

GLTFLoader: Callback for when asset was read #12761

Merged
merged 3 commits into from
Dec 12, 2017
Merged

Conversation

cnspaha
Copy link
Contributor

@cnspaha cnspaha commented Nov 27, 2017

Currently there is absolutely no possibility to access the asset property from the loaded GLTF.
This here is one idea how it could look like. It is similar like "onAfterRender" etc where the developer just have to hook on the function to get the value.

Opinions regarding naming etc are welcome.
Also should we rather call this callback right after checking for the supported version or before? The downside of the current code is that your callback won't be called if the GLTF version isn't supported, but then you normally won't have any need for the information in ```assets``...

Currently there is absolutely no possibility to access the asset property from the loaded GLTF. 
This here is one idea how it could look like. It is similar like "onAfterRender" etc where the developer just have to hook on the function to get the value.

Opinions regarding naming etc are welcome.
Also should we rather call this callback right after checking for the supported version or before?  The downside of the current code is that your callback won't be called if the GLTF version isn't supported, but then you normally won't have any need for the information in ```assets```...
@cnspaha
Copy link
Contributor Author

cnspaha commented Nov 27, 2017

An other possibility (and probably the better) is to put the asset into the returned object

parser.parse( function ( scene, scenes, cameras, animations, asset ) { // also set 'asset'

  console.timeEnd( 'GLTFLoader' );

  var glTF = {
    scene: scene,
    scenes: scenes,
    cameras: cameras,
    animations: animations,
    asset: asset  // this line is new
  };

  onLoad( glTF );
}, onError );

@takahirox
Copy link
Collaborator

Voting for onLoad argument.

@donmccurdy
Copy link
Collaborator

Curious why this is needed? We're hoping to make the loader more extensible in the future; see #11682; I imagine we would revert this change when that happens..

Of the two approaches I do prefer including a property on the returned object, though.

@donmccurdy
Copy link
Collaborator

thinking more about it, I'm happy with the onLoad asset argument idea. Could imagine returning materials that way too eventually, so that it's possible to use a glTF file as a material library.

@cnspaha
Copy link
Contributor Author

cnspaha commented Nov 28, 2017

@donmccurdy
just rewrote it to the onLoad asset variety. please have a look ;)

@donmccurdy
Copy link
Collaborator

@cnspaha there has been an upstream change, could you rebase this branch?

# Conflicts:
#	examples/js/loaders/GLTFLoader.js
@cnspaha
Copy link
Contributor Author

cnspaha commented Dec 7, 2017

@donmccurdy
sorry just didn't accomplished it to rebase this... don't know why git didn't wanted to :/
So I merged down the dev down to my patch-3 branch to resolve the conflicts with the base

@donmccurdy
Copy link
Collaborator

No problem, this looks good. 👍

var cameras = dependencies.cameras || [];

onLoad( scene, scenes, cameras, animations );
onLoad( scene, scenes, cameras, animations, asset );
Copy link
Owner

Choose a reason for hiding this comment

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

What about passing json directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would be possible of course, but then we would pass the whole GLTF data back.
In my opinion this is a bit of overkill so I would stick with the asset. Or are there any other values in the GLTF which would be good to know as caller of the loader? I am not so familiar with the structure of the GLTF format

@mrdoob
Copy link
Owner

mrdoob commented Dec 12, 2017

I see. Well, if @donmccurdy is cool with this I'm cool with this 😊

@mrdoob mrdoob merged commit eab000e into mrdoob:dev Dec 12, 2017
@mrdoob
Copy link
Owner

mrdoob commented Dec 12, 2017

Thanks!

@cnspaha cnspaha deleted the patch-3 branch December 12, 2017 10:09
@donmccurdy
Copy link
Collaborator

What about passing json directly?

This might be worth considering; added a comment on #11682 to revisit later.

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

Successfully merging this pull request may close these issues.

4 participants