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

Instancing models 3d tiles #3059

Merged
merged 28 commits into from
Oct 24, 2015

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Oct 6, 2015

For #3001

First pass of integrating instanced models into 3d-tiles. I'll do a more thorough review later, including cleaning up any TODOs.

}

function loadInstanced3DTile() {
// TODO : create real tileset later
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll be able to delete this entire example since the user's code will just load a 3D tileset regardless of the tile format(s) it contains.

@pjcozzi pjcozzi mentioned this pull request Oct 6, 2015
8 tasks
@@ -6,7 +6,8 @@ define([
'../Core/destroyObject',
'../Core/DeveloperError',
'../Core/IndexDatatype',
'./BufferUsage'
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to get more 3D Tiles code into master sooner, can you open a separate pull request into master for these changes and the changes to ShaderProgram.js?

Copy link
Contributor

Choose a reason for hiding this comment

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

And maybe even the changes to ShaderSource.js (and code that uses it).

@@ -2943,9 +2946,6 @@ define([
BoundingSphere.clone(command.boundingVolume, pickCommand.boundingVolume);
}
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we delete this? We're actually going to want this for KHR_materials_common and when we support glTF cameras.

CC @tfili

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 deleted it because I ended up moving that code to line 2927, which all node types will call.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 14, 2015

@lilleyse just those comments, then this is ready to merge into the 3D Tiles branch.

@lilleyse
Copy link
Contributor Author

This should be ready for another look now.

I made a few modifications to Model in 948ad72 so that ModelInstanceCollections that point to the same model url can reuse most of the resources except VertexArray. Sharing the cached VertexArray was the source of the bugs I had earlier this week, since all collections would also be sharing the same instance data buffer.

I think I discovered the main memory leak you brought up before, but I would like you to verify this.

The rest of the changes are renaming and organizational.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 23, 2015

With instancing disabled in the Sandcastle example, the GC is still a problem:

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 23, 2015

When individual models are used, the GC is still at 10.92% so the bulk (or all) of the allocations could be in Model.js. Check to see if this is in master to help narrow it down. In either case, we'll want to fix it.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 23, 2015

@lilleyse we can merge this once we determine the allocations.

As I mentioned before, I also think we can combine BatchedModel and InstancedModel but let's see how 3D Tiles evolve a bit more first.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 24, 2015

Moved the GC issues to #3125.

pjcozzi added a commit that referenced this pull request Oct 24, 2015
@pjcozzi pjcozzi merged commit 1a62043 into CesiumGS:3d-tiles Oct 24, 2015
@pjcozzi pjcozzi deleted the instancing-models-3d-tiles branch October 24, 2015 12:29
@laurensdijkstra
Copy link

What is the status of Instanced glTF models? When checking the source code of Cesium master I could not find any reference to instanceCount or ModelInstanceCollection. We would like to draw alot of trees, but the performance is pretty bad.

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 30, 2016

@laurensdijkstra model instancing is in the 3d-tiles branch.

@laurensdijkstra
Copy link

Thanks @pjcozzi !

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