Skip to content

Options to convert up axis#68

Merged
likangning93 merged 2 commits intomasterfrom
up-axis
Apr 25, 2017
Merged

Options to convert up axis#68
likangning93 merged 2 commits intomasterfrom
up-axis

Conversation

@lilleyse
Copy link
Copy Markdown
Contributor

Fixes #63

@likangning93 can you review and test?

Comment thread README.md
|`--checkTransparency`|Do a more exhaustive check for texture transparency by looking at the alpha channel of each pixel. By default textures are considered to be opaque.|No, default `false`|
|`--secure`|Prevent the converter from reading image or mtl files outside of the input obj directory.|No, default `false`|
|`--inputUpAxis`|Up axis of the obj. Choices are 'X', 'Y', and 'Z'.|No, default `Y`|
|`--outputUpAxis`|Up axis of the converted glTF. Choices are 'X', 'Y', and 'Z'.|No, default `Y`|
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Doesn't the glTF spec mandate Y up?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah...

We also go against the spec in 3d-tiles by allowing z-up gltfs. Personally I don't see too much harm.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That makes sense I guess, thanks.

Copy link
Copy Markdown
Contributor

@likangning93 likangning93 left a comment

Choose a reason for hiding this comment

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

Setting Z-up for input seems to work like a charm for a couple models I have. Also, using a matrix multiply instead of fiddling with swizzling doesn't seem to lead to a noticeable performance hit, even on 1 million+ vertices. Awesome!

Just some speculation on testing, but I don't think I see anything that needs to be addressed in another code change.

Comment thread specs/lib/loadObjSpec.js
expect(rotatedNormal).toEqual(normal);
} else {
expect(rotatedPosition).not.toEqual(position);
expect(rotatedNormal).not.toEqual(normal);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess it's not a big deal since we're using Cesium's up-axis conversion, which I assume is relatively well tested, but should we try to come up with more definitive tests for this? It could be a huge pain though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually these types of tests are missing from Cesium too... I think they do belong there instead of here.

@likangning93 likangning93 merged commit 930b14a into master Apr 25, 2017
@likangning93 likangning93 deleted the up-axis branch April 25, 2017 14:15
@mramato
Copy link
Copy Markdown
Contributor

mramato commented Apr 25, 2017

@lilleyse can we do a release today so we have this and the ambient fix published. Thanks.

@lilleyse
Copy link
Copy Markdown
Contributor Author

Published.

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