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

Update CHANGES for model orientation change #6738

Merged
merged 4 commits into from
Jul 2, 2018
Merged

Conversation

lilleyse
Copy link
Contributor

Fixes #6713 by giving instructions about how to correct models that are rotated unexpectedly.

@cesium-concierge
Copy link

Signed CLA is on file.

@lilleyse, thanks for the pull request! Maintainers, we have a signed CLA from @lilleyse, so you can review this at any time.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@hpinkos
Copy link
Contributor

hpinkos commented Jun 27, 2018

@lilleyse is this transformation something we apply runtime in Cesium or is it saved as part of the glTF? Can we have more specific instructions for how to make this change? I wouldn't know how to do it from what you wrote.

@lilleyse
Copy link
Contributor Author

It's saved into the glTF. I updated the wording, hopefully it is clearer now.

@hpinkos
Copy link
Contributor

hpinkos commented Jun 27, 2018

Which one is the root node? Is it always nodes[0]?
What do I do for my glb models?

Would something like "open up the source model in a model editor and rotate it to face the correct axis and re-convert it to glTF" be an alternate recommended solution?

The instructions need to be clear to someone who has little to no understanding of the glTF spec. Asking someone to open the glTF in a text editor isn't a good solution for someone with less of a technical background. And it won't work at all if that matrix has a value already, right?

@likangning93
Copy link
Contributor

@lilleyse @hpinkos is providing a client-side workaround for "wrong" models ok? It'd be more self-contained than, say, developers having to hand-edit the glTF or ask artists for new assets.
It seems like for most Entity/CZML cases this could just be solved by adjusting the entity orientation:

    var position = Cesium.Cartesian3.fromDegrees(-123.0744619, 44.0503706, height);
    var heading = Cesium.Math.toRadians(135); // add/subtract 90 degrees
    var pitch = 0;
    var roll = 0;
    var hpr = new Cesium.HeadingPitchRoll(heading, pitch, roll);
    var orientation = Cesium.Transforms.headingPitchRollQuaternion(position, hpr);

    var entity = viewer.entities.add({
        position : position,
        orientation : orientation,
        model : {
            uri : '../../SampleData/models/CesiumAir/Cesium_Air.glb',
            minimumPixelSize : 128,
            maximumScale : 20000
        }
    });

And then in the Primitive API users probably just need to premultiply a rotation onto the model matrix or something.

@mramato
Copy link
Contributor

mramato commented Jun 29, 2018

I don't think a Cesium workaround is a good idea because the model is still "wrong".

I'm with @lilleyse on this one. Cesium was incorrect before and that led to equally incorrect models. We should advise everyone to fix their models.

@lilleyse
Copy link
Contributor Author

Yeah, models that are definitely incorrect should be fixed. E.g. the ground vehicle was +X forward and was fixed to be +Z forward. I don't think I would consider @kladess's models in #6713 incorrect though... which is why I don't really like the suggestion in CHANGES.md all that much. I think the CZML update would actually be a lot easier. But I guess if we had to pick one thing to go in CHANGES.md, it would be to update the model. Now time to get the wording right...

@likangning93
Copy link
Contributor

likangning93 commented Jun 29, 2018

We should advise everyone to fix their models.

I still think we should have a client-side fix documented in some capacity.
What about for users that don't have the option to fix their models?
Maybe the glbs are being loaded from a customer's server and the customer refuses to fix the models, or the point-of-contact is on vacation until after a deadline, or a myriad of other frustrating reasons.

@mramato
Copy link
Contributor

mramato commented Jun 29, 2018

I still think we should have a client-side fix documented in some capacity.

OK, if @lilleyse agrees with this I'm fine with it.

@lilleyse lilleyse force-pushed the changes-md-update branch 2 times, most recently from e50145f to faaf328 Compare June 29, 2018 19:05
@lilleyse
Copy link
Contributor Author

Updated. I tried to cover all the cases here, but didn't want to get too verbose. I linked back to this PR where we can provide more instructions if needed.

CHANGES.md Outdated
@@ -4,7 +4,9 @@ Change Log
### 1.47 - 2018-07-02

##### Breaking Changes :mega:
* glTF 2.0 models corrected to face +Z forwards per specification. Internally Cesium uses +X as forward, so a new +Z to +X rotation was added for 2.0 models only. [#6632](https://github.com/AnalyticalGraphicsInc/cesium/pull/6632)
* glTF 2.0 models corrected to face +Z forwards per specification. Internally Cesium uses +X as forward, so a new +Z to +X rotation was added for 2.0 models only. To fix models that are oriented incorrectly after this change:
* If the model faces +X forwards update the glTF to face +Z forwards. This can be done by loading the glTF in a model editor and applying a -90 degree rotation about the Y-axis. Alternatively, add a new root node to the glTF node hierarchy whose `matrix` is `[0,0,1,0,0,1,0,0,-1,0,0,0,0,0,0,1]`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Some modeling programs might have a different up/forward convention, so maybe it's better to specify something like a 90 degree clockwise rotation about the Up-axis.

@lilleyse
Copy link
Contributor Author

lilleyse commented Jul 2, 2018

Thanks @likangning93, updated.

@likangning93 likangning93 merged commit a083f08 into master Jul 2, 2018
@likangning93 likangning93 deleted the changes-md-update branch July 2, 2018 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants