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.

Sign up for GitHub

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

Jump to bottom

Missed a constructor in AnalyticalGraphicsInc/cesium#6632. #6681

Merged
merged 3 commits into from
Jun 13, 2018
Merged

Missed a constructor in AnalyticalGraphicsInc/cesium#6632. #6681

merged 3 commits into from
Jun 13, 2018

Conversation

emackey
Copy link
Contributor

@emackey emackey commented Jun 13, 2018

Addresses #6632 (comment)

@cesium-concierge
Copy link

Signed CLA is on file.

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

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

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

🌍 🌎 🌏

@emackey emackey requested a review from lilleyse June 13, 2018 01:01
lilleyse
lilleyse reviewed Jun 13, 2018
View reviewed changes
Source/Scene/Instanced3DModel3DTileContent.js
@@ -281,6 +283,7 @@ define([
basePath : undefined,
incrementallyLoadTextures : false,
upAxis : content._tileset._gltfUpAxis,
forwardAxis : Axis.X,
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need to get passed along through ModelInstanceCollection as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a test case, in Sandcastle maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

The test tilesets all use boxes... but you can replace box.glb in this tileset with something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Was able to reproduce and confirm the fix here too. Looks like travis failed, I'll look into that next.

@lilleyse
Copy link
Contributor

Looks good from my end -

master

before2

1.46 and this PR:

after2

@emackey
Copy link
Contributor Author

emackey commented Jun 13, 2018

No idea what's up with travis.

@mramato
Copy link
Contributor

mramato commented Jun 13, 2018

No idea what's up with travis.

Travis is just flaky. Looks like everything is passing now.

@lilleyse lilleyse merged commit c3f5a51 into master Jun 13, 2018
@lilleyse lilleyse deleted the more-orientations branch June 13, 2018 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lilleyse lilleyse lilleyse left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@emackey @cesium-concierge @lilleyse @mramato