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

ColladaLoader2: Added axis conversion support #11404

Closed
wants to merge 5 commits into from
Closed

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented May 27, 2017

This PR introduces axis conversion support for ColladaLoader2. So when the user sets the option .convertUpAxis to true, the loader will automatically convert the data to the three.js standard (Y_UP).

The example shows the default Blender starter scene (mesh, point light, camera, Z_UP) with correct position and orientation.

switch ( asset.upAxis ) {

case 'X_UP':
asset.upConversion = 'X-Y';
Copy link
Owner

@mrdoob mrdoob May 30, 2017

Choose a reason for hiding this comment

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

How about doing this instead?

switch ( asset.upAxis ) {

	case 'X_UP':
		convertVector3 = function ( vector, sign ) {
			if ( sign === undefined ) sign = - 1;
			var tmp = vector.x;
			vector.x = sign * vector.y;
			vector.y = tmp;
			return vector;
		};
		break;

	case 'Y_UP':
		convertVector3 = function ( vector, sign ) {
			return vector;
		};
		break;

	case 'Z_UP':
		convertVector3 = function ( vector, sign ) {
			if ( sign === undefined ) sign = - 1;
			var tmp = vector.y;
			vector.y = vector.z;
			vector.z = sign * tmp;
			return vector;
		};
		break;
}

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 30, 2017

Love this change! 😊

@WestLangley
Copy link
Collaborator

@Mugen87 Did you confirm that rotations work correctly when axes are flipped?

Also, are these conversions a simple rotation of the coordinate system, or do they involve a conversion from a left-handed system to a right-handed one?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 30, 2017

I've tested with different setups of the blender startup scene (so different rotations of the cube and camera positions). The exported file has Z_UP configured and was correctly loaded by ColladaLoader2.

According to the spec (see screenshot), all coordinates of a collada file are right-handed.

image


var scene = parseScene( getElementsByTagName( collada, 'scene' )[ 0 ] );

if ( asset.upAxis === 'Z_UP' ) {

scene.rotation.x = - Math.PI / 2;
Copy link
Owner

Choose a reason for hiding this comment

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

Here's another though...

Instead of modifying all the vertices, normals, matrices... would it be possible to just rotate the resulting scene? We could then add "flatten" methods if the user needed to convert...?

Copy link
Collaborator Author

@Mugen87 Mugen87 May 31, 2017

Choose a reason for hiding this comment

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

It think this approach should be okay in most cases. But let's say you use OrbitControls and attach a camera object from the collada scene. Then you will get something like this: https://yume.human-interactive.org/examples/collada_current/

There might be more scenarios where similar unexpected behaviors occur. The only way to avoid such problems is a true conversion of all geometry and animation data. And i also think that this solution avoids many support request/bug reports.

We could then add "flatten" methods if the user needed to convert...?

How would this look like?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Mugen87 I still do not see how your code can be correct when you do not "convert" rotations -- but I admit, I may be missing something.

In any event, I agree with @mrdoob, that it may be better to have one method that converts the entire hierarchy at the end, rather than doing it as-you-go. If it is always a simple rotation of the coordinate system, then it should be straight-forward.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps THREE.SceneUtils.convertFromZUp( root );. It would be understood that the method would convert "to y-up".

Maybe, convertToZUp(), too, for exporting. I am not aware of other coordinate systems in use, unless you would want to support left-handed ones. That is a bit messy, but it would make such a method even more useful.

Copy link
Owner

@mrdoob mrdoob May 31, 2017

Choose a reason for hiding this comment

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

Yeah, converting as you go, even if more efficient, tends to get hairy quickly... The blender exporter may be a good example of that 🙁

Copy link
Collaborator

Choose a reason for hiding this comment

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

All true. If the blender exporter -- or any exporter -- is rewritten, it should avoid piecemeal conversions, and also Euler angles. Use quaternions or matrices, instead.

I do think a utility to convert to/from the three.js coordinate system could be useful.

Copy link
Collaborator Author

@Mugen87 Mugen87 May 31, 2017

Choose a reason for hiding this comment

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

@WestLangley What part of the code is not correct from your point of view? And can you please define what you understand under "convert rotations" ? I thought the code does this actually (sry, i'm a little bit uncertain about this topic now).

Just for my understanding 😊 : If we implement a generic convertFromZUp() function, it needs to regard the entire hierarchy of objects and not only the root object, right? And we also have to convert geometry and animation data, correct?

Copy link
Owner

Choose a reason for hiding this comment

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

Correct 👌

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Mugen87 Maybe your handling of rotations was fine. I admit, I have not had the time to think about it sufficiently. Anyway, if you are taking a different approach now, it does not matter, I guess.... Also, the generic convertToZUp() could just involve the root -- I think...

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 31, 2017

Closing. This code will be implemented in a separate entity and not inside the loader.

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.

3 participants