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

Support instanced models in 2D #4996

Merged
merged 3 commits into from Feb 13, 2017
Merged

Support instanced models in 2D #4996

merged 3 commits into from Feb 13, 2017

Conversation

lilleyse
Copy link
Contributor

For #3241

I tested out a variety of cases like regular i3dm tiles, i3dm with tile transform / changing the transform dynamically, 3D Models Instancing sandcastle with instancing on/off.

The approach here transforms the instances to 2D with a transform derived from the collection's center.

I think this approach will be fine for most uses cases, like most i3dm tiles, but will definitely break down if instances are located all across the globe. For that the getRtcTransform2D function would have to be on an instance-by-instance basis and the vertex buffer would need to be updated when switching to 2D. Since it is a one time operation it may be worth just doing that.

2d

@pjcozzi pjcozzi mentioned this pull request Feb 13, 2017
53 tasks
@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 13, 2017

Part of #4678

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 13, 2017

CC @pmconne

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 13, 2017

Since it is a one time operation it may be worth just doing that.

Sounds OK to me.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 13, 2017

@bagnell can you please review and merge when happy?

@lilleyse
Copy link
Contributor Author

I thought about my comments some more and I think handling that case can pass for now. Even with the fixes for 2D there would still be issues with 3D since it renders instances relative to center. To fix that completely (using positionMin and positionMax I'm guessing) would be a bigger architectural change that can wait for now.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 13, 2017

OK, please make sure the limitation is in the reference doc.

var scratchMatrix = new Matrix4();
var scratchRotation = new Matrix3();

function getRtcTransform2D(frameState, rtcTransform, result) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This does the same thing as result = Transforms.basisTo2D(frameState.mapProjection, rtcTransform, result);. Why don't you use that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wish I realized this ahead of time... anyways is the version here functionally equal to the Transforms.basisTo2D, could that be simplified with the code here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that would be great. That function does the swizzling manually where this does it with a matrix multiply. It also ensures that the rotation matrix is orthogonal, but that shouldn't be necessary.

@bagnell
Copy link
Contributor

bagnell commented Feb 13, 2017

This looks good. Just that one comment.

Something unrelated to this PR: why is the camera minimum zoom set for the example?

@lilleyse
Copy link
Contributor Author

Something unrelated to this PR: why is the camera minimum zoom set for the example?

I must have just modeled it after the 3D Models demo when I first wrote it. But it's not needed so I removed it.

@lilleyse
Copy link
Contributor Author

Updated.

@bagnell bagnell merged commit 1c5c326 into 3d-tiles Feb 13, 2017
@bagnell bagnell deleted the mic-2d branch February 13, 2017 22:43
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.

None yet

3 participants