Model Instancing #3049

lilleyse · 2015-09-28T18:54:47Z

To do:

Handle adding and removing instances
Handle updating instances
Test that picking works
Fix precision jittering
Performance measurements
Add specs
Frustum culling instances (optional)
@pjcozzi - add this to the glTF spec: "This assumes techniques that share a program do not declare different semantics for the same uniforms."

pjcozzi · 2015-09-28T18:57:58Z

Apps/Sandcastle/gallery/development/3D Models Instancing.html

+
+createCollection('../../SampleData/models/CesiumAir/Cesium_Air.bgltf', 10);
+
+//function createModel(url, height) {


We don't leave in commented out code. Should we delete this?

I use this code for testing sometimes, and I will delete it once I'm further along with testing. In fact I'll probably need it back for the performance testing.

pjcozzi · 2015-09-28T19:12:19Z

Source/Scene/Model.js

@@ -796,6 +842,8 @@ define([
     * @param {Number} [options.minimumPixelSize=0.0] The approximate minimum pixel size of the model regardless of zoom.
     * @param {Boolean} [options.allowPicking=true] When <code>true</code>, each glTF mesh and primitive is pickable with {@link Scene#pick}.
     * @param {Boolean} [options.asynchronous=true] Determines if model WebGL resource creation will be spread out over several frames or block until completion once all glTF files are loaded.
+     * @param {Boolean} [options.instanced=false] Whether the model will be instanced.


Same comment as above.

pjcozzi · 2015-09-28T19:20:37Z

Add tests to the task list for this pull request.

pjcozzi · 2015-09-28T19:26:33Z

Source/Scene/ModelInstanceCollection.js

+    };
+
+    /**
+     * A 3D model instance collection. All instances reference the same underlying model, but have unique


Let's mark this @private until we hook it up to 3D Tiles. This way we can change it if needed. We probably won't merge it until then either, but we can still merge billboard instancing.

pjcozzi · 2015-09-28T19:28:30Z

Source/Scene/ModelInstanceCollection.js

+
+        this._model = undefined;
+        this._boundingSphere = undefined;
+        this._instancingEnabled = false;


Perhaps _instancingSupported. I had to look at all of the code to realize what this is for.

pjcozzi · 2015-09-28T19:30:20Z

I suggest we hook this up to 3D Tiles before doing the following items in your task list:

Handle adding and removing instances
Handle updating instances
Frustum culling instances (optional)

We would still do the others beforehand.

pjcozzi · 2015-09-28T19:31:29Z

Source/Scene/ModelInstanceCollection.js

+    };
+
+    defineProperties(ModelInstanceCollection.prototype, {
+        model : {


What is this getter used for?

pjcozzi · 2015-09-28T19:37:43Z

This is off to a good start, but I am not sure that ModelInstanceCollection and Model and factored correctly; it feels like we had to put too many changes into Model so it is losing cohesion. Let's brainstorm more on if this is the right approach.

pjcozzi · 2015-09-28T19:38:45Z

Apps/Sandcastle/gallery/development/3D Models Instancing.html

+        var position = new Cesium.Cartesian3.fromDegrees(longitude, latitude, height + i * 20);
+
+        var modelMatrix = Cesium.Transforms.headingPitchRollToFixedFrame(position, heading, pitch, roll);
+        instances.push({


We'll need an explicit ModelInstance type as we flush out picking, show/hide, etc.

lilleyse · 2015-09-28T21:59:55Z

I was able to fix the jittering issues by trying something you mentioned earlier. I created a uniform for the modelView matrix of the collection's bounding volume. Then I modified each instance's model matrix to be an offset from the center of the bounding volume.

This will work fine for 3d tiles at least. For larger regions, I could see us doing a similar approach - finding a bounding volume for the visible set of objects. Or just subdividing collections before hand, or stream the instance modelView matrices.

I still need to do all the refactoring, so you don't have to check this code yet.

pjcozzi · 2015-09-28T22:31:47Z

Great news. When zoomed in close, this should be good for ~131,071 meter spacing. See http://blogs.agi.com/insight3d/index.php/2008/09/03/precisions-precisions/

For instanced satellite models, for example, we would probably just stream the model matrices. For static models, 3D Tiles implicitly provides subdivision.

lilleyse · 2015-09-30T13:53:08Z

Updated so that all the instancing code out of Model.

pjcozzi · 2015-09-30T15:10:51Z

@lilleyse can you please update the task list at the top of this PR?

pjcozzi · 2015-09-30T15:13:27Z

Source/Scene/Model.js

@@ -772,6 +781,10 @@ define([
        return JSON.parse(json);
    }

+    Model.getDefaultCacheKey = function(url) {


Why put this on Model instead of just having a file-scoped function? Do you plan to use this in the tests?

I'm calling it in ModelInstanceCollection as well. I'm not really happy with this either.

Ah, I missed that. I think it is fine; I'll let you know if I come up with anything else.

In the meantime, explicitly mark it @private.

At the very least, it should be Model._getDefaultCacheKey and be marked @private. But why not just have ModelInstanceCollection access the _url property directly? Can't _url be undefined if the model was loaded from a JSON object?

When making the cache key Model converts the url to an absolute url, otherwise I would use _url directly. I'm handling the case where ModelInstanceCollection creates the Model from JSON or binary now.

pjcozzi · 2015-09-30T15:21:13Z

Source/Scene/Model.js

@@ -306,6 +306,16 @@ define([
        this._releaseGltfJson = defaultValue(options.releaseGltfJson, false);
        this._animationIds = undefined;

+        // Undocumented options


I can help you set up some test data for when we merge this into the 3d-tiles branch and need to confirm it still works after fixing the merge conflicts.

pjcozzi · 2015-09-30T15:59:46Z

Source/Scene/ModelInstanceCollection.js

+        for (i = 0; i < commandsLength; ++i) {
+            var modelCommand = modelCommands[i];
+            for (j = 0; j < instancesLength; ++j) {
+                var commandIndex = i*instancesLength+j;


We always use whitespace, e.g., i * instancesLength + j

pjcozzi · 2015-09-30T16:02:45Z

Source/Scene/ModelInstanceCollection.js

+            return;
+        }
+
+        if (this._show === false) {


if (!this._show) {

pjcozzi · 2015-09-30T17:36:27Z

Source/Scene/ModelInstanceCollection.js

+        var model = this._model;
+        model.update(context, frameState, []);
+
+        if (model._ready && (this._state === LoadState.LOADING)) {


Use model.ready.

pjcozzi · 2015-09-30T18:16:08Z

This is getting into pretty good shape. I'll make another pass over it once we flush it out by integrating it with 3D Tiles and writing the tests.

pjcozzi · 2015-10-06T15:23:14Z

The rest of this review is in #3059, which is a pull request into the 3d-tiles branch.

lilleyse and others added 2 commits September 28, 2015 14:53

Added Model instancing

d634668

Whitespace

bdaafc7

pjcozzi reviewed Sep 28, 2015
View reviewed changes

Whitespace

858fc15

pjcozzi reviewed Sep 28, 2015
View reviewed changes

Whitespace

38106e6

pjcozzi reviewed Sep 28, 2015
View reviewed changes

Tweak doc

b1142b3

pjcozzi reviewed Sep 28, 2015
View reviewed changes

Fixed jittering by rendering relative to bounding volume

3019bdb

Moved all instancing code into ModelInstanceCollection

5d77f8f

Small fix

7868753

Tweak comment

e740dfd

pjcozzi reviewed Sep 30, 2015
View reviewed changes

Style tweak

8a79a96

pjcozzi reviewed Sep 30, 2015
View reviewed changes

Style tweak

783ffe7

pjcozzi reviewed Sep 30, 2015
View reviewed changes

Style tweak

dedb207

pjcozzi reviewed Sep 30, 2015
View reviewed changes

Source/Scene/ModelInstanceCollection.js

return;

}

if (this._show === false) {

Copy link

Contributor

pjcozzi Sep 30, 2015

Choose a reason for hiding this comment

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

if (!this._show) {

pjcozzi mentioned this pull request Sep 30, 2015

Instanced 3D Model and Composite Tile Formats CesiumGS/3d-tiles#23

Closed

5 tasks

pjcozzi reviewed Sep 30, 2015
View reviewed changes

lilleyse added 2 commits September 30, 2015 14:29

Updated model instancing Sandcastle demo

4964def

Minor fixes

99a959d

pjcozzi closed this Oct 5, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Model Instancing #3049

Model Instancing #3049

lilleyse commented Sep 28, 2015

pjcozzi Sep 28, 2015

lilleyse Sep 28, 2015

pjcozzi Sep 28, 2015

pjcozzi commented Sep 28, 2015

pjcozzi Sep 28, 2015

pjcozzi Sep 28, 2015

pjcozzi commented Sep 28, 2015

pjcozzi Sep 28, 2015

pjcozzi commented Sep 28, 2015

pjcozzi Sep 28, 2015

lilleyse commented Sep 28, 2015

pjcozzi commented Sep 28, 2015

lilleyse commented Sep 30, 2015

pjcozzi commented Sep 30, 2015

pjcozzi Sep 30, 2015

lilleyse Sep 30, 2015

pjcozzi Sep 30, 2015

mramato Sep 30, 2015

lilleyse Sep 30, 2015

pjcozzi Sep 30, 2015

pjcozzi Sep 30, 2015

pjcozzi Sep 30, 2015

pjcozzi Sep 30, 2015

pjcozzi commented Sep 30, 2015

pjcozzi commented Oct 6, 2015


		createCollection('../../SampleData/models/CesiumAir/Cesium_Air.bgltf', 10);

		//function createModel(url, height) {

Model Instancing #3049

Model Instancing #3049

Conversation

lilleyse commented Sep 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Sep 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Sep 28, 2015

Choose a reason for hiding this comment

pjcozzi commented Sep 28, 2015

Choose a reason for hiding this comment

lilleyse commented Sep 28, 2015

pjcozzi commented Sep 28, 2015

lilleyse commented Sep 30, 2015

pjcozzi commented Sep 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Sep 30, 2015

pjcozzi commented Oct 6, 2015