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

ColladaLoader: Added possibility to manually set path #12614

Merged
merged 4 commits into from
Nov 22, 2017

Conversation

cnspaha
Copy link
Contributor

@cnspaha cnspaha commented Nov 9, 2017

var path = THREE.Loader.prototype.extractUrlBase( url );


scope.path = scope.path === undefined ? THREE.Loader.prototype.extractUrlBase( url ) : scope.path;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would make this:

var path = scope.path === undefined ? THREE.Loader.prototype.extractUrlBase( url ) : scope.path;

Otherwise consider this case:

var loader = new THREE.ColladaLoader();
loader.load('path/to/first/model.dae', () => ...);
loader.load('other/path/to/second/model.dae', () => ...);

The first model will modify scope.path and break the second model.

Copy link
Collaborator

@Mugen87 Mugen87 Nov 9, 2017

Choose a reason for hiding this comment

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

If we do it like this, we also handle the path for textureLoader correctly, see

https://github.com/cnspaha/three.js/blob/18b700af3c3e50f82a8112f54061af212db8011e/examples/js/loaders/ColladaLoader.js#L3367

@cnspaha The path variable at this place is passed as a parameter of parse(). With your current implementation, this value isn't set correctly.

Copy link
Collaborator

@Mugen87 Mugen87 Nov 9, 2017

Choose a reason for hiding this comment

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

I think it would be the best to handle this in the same way like GLTFLoader does.

Copy link
Contributor Author

@cnspaha cnspaha Nov 10, 2017

Choose a reason for hiding this comment

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

@donmccurdy
I get your point.
Just saw, that I pretty messed up this one...
I like your idea, thanks again for your input ;)

@Mugen87
The suggestion of @donmccurdy is exactly what the GLTFLoader does, isn't it?

edit: commited it

var loader = new THREE.FileLoader( scope.manager );
loader.setPath( scope.path );
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is not necessary. Everything else is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.


var path = scope.path === undefined ? THREE.Loader.prototype.extractUrlBase( url ) : scope.path;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sry for being so pedantic. But it seems you used soft tabs here. Please ensure correct code style by using hard tabs instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't worry about it, you are totaly right ;)
didn't pay attention to this one

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 10, 2017

Looks good now! Thanks!

@cnspaha
Copy link
Contributor Author

cnspaha commented Nov 14, 2017

@mrdoob
can you merge this please? Thank you :)

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 18, 2017

@mrdoob This one can be safely merged 😊

@mrdoob
Copy link
Owner

mrdoob commented Nov 22, 2017

Sorry for the delay guys!

@mrdoob mrdoob merged commit 1477d72 into mrdoob:dev Nov 22, 2017
@mrdoob
Copy link
Owner

mrdoob commented Nov 22, 2017

Thanks!

@cnspaha cnspaha deleted the patch-6 branch November 22, 2017 06:17
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