Add back ModelInstance class

[lukemckinstry]

Description

• Adds back a ModelInstance Class for runtime instancing.
• Refactor of ModelSceneGraph to resolve issues in how transforms were applied to models with multiple hierarchical nodes.

ModelInstance and ModelInstanceCollection

Supply transformation matrices (Matrix4) to the Model for each instance by

• calling model.instance.add(transform)
• supplying options.instances to Model.loadGltfAsync()

In this way, the transformation matrix for each instance can be computed from a cartographic coordinate as shown below.

Example:

const viewer = new Cesium.Viewer("cesiumContainer");

const model = await Cesium.Model.fromGltfAsync({
    url: "../../SampleData/models/GroundVehicle/GroundVehicle.glb",
  });
const lng = -75.1652;
const lat = 39.9526;
const height = 30.0;

let position;
let transform;
function genInstances(count) {
  for (let i = 0; i < count; i++) {
    position = Cesium.Cartesian3.fromDegrees(
      lng + i * Math.random() * 0.0001,
      lat + i * 0.0001,
      height + i,
    );
    transform = new Cesium.Transforms.eastNorthUpToFixedFrame(
      position,
    );
    model.instances.add(transform);
  }
}
viewer.scene.camera.setView({
  destination: Cesium.Cartesian3.fromDegrees(lng, lat, height),
  orientation: new Cesium.HeadingPitchRoll(
    2.205737333179613,
    -0.7255022564055849,
    6.283181225638178,
  ),
});

Shader logic

Cesium already supported instancing from glTF (specifies instancing in model space) and i3DM (specifies instancing in world space). Because this feature wanted to set up instancing in world space, we followed i3DM logic, including for the shader.

i3DM

positionMC = instance * node * position

node includes axis correction ie.
node = (axis correction * node) 

view * model * Scene Graph top level * instance * node * position

Runtime instancing

Divide instance into instance_center and instance_relative_to_center_transform

instance_relative = instance_relative_to_center_transform * Scene graph top level * axis correction * node

The instance center is calculated relative to eye:

instance = instance_center +translate+ instance_relative

Then modelView is set to czm_modelViewRelativeToEye (transforms model coordinates, relative to the eye, to eye coordinates)

czm_modelViewRelativeToEye * instance

ModelSceneGraph refactor

ToDo:

• broken tests (mostly classes impacted by the scene graph transforms refactor)
  • ModelSceneGraph
  • ModelDrawCommands
• Add tests
  • ModelInstance Class
  • RuntimeModelInstancingPipelineStage
  • ModelInstancesUpdateStage
• document edge case restrictions, have not yet restricted these in code
  • [ ]When instances are supplied to Model, modelMatrix must be the identify matrix. (this is supported)
  • Cannot supply instances to a model with glTF with ext_mesh_gpu_instance extension, throw developer error
  • 2D and columbus view - ModelInstanceCollection 2D Scene support #12816
• Clean up sandcastle example
• demo picking in sandcastle
• document important technical details on issue - including scene graph and bounding sphere calculation refactors

Performance notes

• Users should optimize the models they are instancing with a tool like gltf-transform optimizegltf-transform optimize wind_turbine_small.glb wind_turbine_optimize.glb --texture-compress webpor glTF Report
• Applying model.scale has moderate performance cost, scale models offline beforehand if this impacts performance
  • not difficult to optimize
• Apply model.minimumPixelSize has major performance cost, only use if needed
  • possible to optimize, ideally if we could track when the camera has moved

Issue number and link

#10846

Testing plan

• General
  • Test turbine sandcastle example Apps/Sandcastle/gallery/3D Model Instancing Terrain.html
• Instances transformed, added, or removed
  • Test development sandcastle example Apps/Sandcastle/gallery/3D Model Instancing.html
    • In setTimeout loop instance transforms will be changed
• API options
  • add instances in Model.fromGltfAsync
    • Use link A, point model url to non-instanced glTF eg. GroundVehicle
  • add instances via ModelInstanceCollection.add
    • Use link B, point model url to non-instanced glTF eg. GroundVehicle
• Earth centered vs alternate center
  • Earth centered - turbine example
  • non Earth centered - Link A
• Model scaling
  • Test model.minimumPixelSize - use 3D Model Instancing Terrain sandcastle
    • instances should scale accordingly, same behavior as for single model
  • Test model.scale
    • instances should scale accordingly
• Performance
  • 50,000 US wind turbines link
• glTF formats
  • glTF w/ single node - CesiumBalloon
  • glTF w/ hierarchical node tree - GroundVehicle, CesiumMilkTruck
  • glTF w/ top level transform
  • glTF with Ext_mesh_gpu_instancing - should throw runtime error
    • instances supplied to Model.fromGltfAsync link A
    • ModelInstanceCollection.add called after model created link B
• Picking - see Picking for runtime instancing with ModelInstance class #12744
• Map views
  • 2D
  • Columbus View
• Review regression other sandcastles
  • 3D Models

Author checklist

• I have submitted a Contributor License Agreement
• I have added my name to CONTRIBUTORS.md
• I have updated CHANGES.md with a short summary of my change
• I have added or updated unit tests to ensure consistent code coverage
• I have updated the inline documentation, and included code examples where relevant
• I have performed a self-review of my code

[github-actions]

Thank you for the pull request, @lukemckinstry!

✅ We can confirm we have a CLA on file for you.

[ggetz]

Thanks for getting this updated and itemized @lukemckinstry! Just a few notes on what we should make sure to test.

Since the workflow was touched by some refactoring, we should confirm the following are still working for non-instances models:

• Clamp to ground: Viz, zooming to, picking
• 2D Mode: Viz, zooming to, projectTo2D, picking
• Animations

We also need to check scaling with minimumPixelSize and maximumScale for both regular models and model instances.

[lukemckinstry]

Since the workflow was touched by some refactoring, we should confirm the following are still working for non-instances models:

• Clamp to ground: Viz, zooming to, picking
• 2D Mode: Viz, zooming to, projectTo2D, picking
• Animations

All these are resolved.

We also need to check scaling with minimumPixelSize and maximumScale for both regular models and model instances.

Scaling is fixed for regular models, but not yet implemented for runtime instanced models. and implemented for runtime instanced models.

[lukemckinstry]

@ggetz

1. Confirming two restrictions in the runtime model instancing feature

• When instances are supplied to Model, modelMatrix must be the identify matrix.
• Cannot supply instances to a model with glTF with ext_mesh_gpu_instance extension, throw developer error

I did not add these to the code yet

2. Also confirming a detail related to making model.instances depend on a collection class so we have build in add and remove functions. Right now, model.instances is just a pass through to ModelSceneGraph._modelInstances, so is my understanding correct that the new collection should be in ModelSceneGraph._modelInstances and not model.instances?

[ggetz]

When instances are supplied to Model, modelMatrix must be the identify matrix.

Yes. Let's (a) ignore any other value that it might be set to, and (b) document this behavior in model instances property.

Cannot supply instances to a model with glTF with ext_mesh_gpu_instance extension, throw developer error

Yes, although a RuntimeError may make more sense then a developer error– The developer may not know all the models which will be loaded into their application until runtime. Again, this restriction should be documented.

[ggetz]

Also confirming a detail related to making model.instances depend on a collection class so we have build in add and remove functions. Right now, model.instances is just a pass through to ModelSceneGraph._modelInstances, so is my understanding correct that the new collection should be in ModelSceneGraph._modelInstances and not model.instances?

I think that would be fine. ModelSceneGraph is an implementation detail that should not be accessible by the user. The import things are that (a) the .add function and similar functionality of the collection is accessible from the public Model interface and (b) the needed information is passed through to ModelSceneGraph.

[ggetz]

@lukemckinstry Once the public API is where you'd like it, a reminder to clean up the Sandcastle example.

[lukemckinstry]

Apps/Sandcastle/gallery/3D Model Instancing Terrain.html is the sandcastle I think is best to release. This has a new wind turbine glb added to the SampleData folder. I believe the CC license makes using it ok #12588 (comment)

[ggetz]

@lukemckinstry Thanks for resolving the last few features!

I looked over the unit tests so far and left a few comments. Let me know if there are any concerns around those.

[ggetz]

It looks like this test is jumping through a few hoops to validate the buffer contents.

Instead of creating a new buffer, perhaps consider validating the output of RuntimeModelInstancingPipelineStage._getTransformsTypedArray against expectedTransformsTypedArray, and then checking runtimeNode.instancingTransformsBuffer's usage and byteLength properties directly.

[lukemckinstry]

I thought the crux of this test should be to make sure that when ModelInstancesUpdateStage.update runs, if sceneGraph.modelInstances._instances has changed, runtimeNode.instancingTransformsBuffer is updated accordingly.

The output of RuntimeModelInstancingPipelineStage._getTransformsTypedArray (when run on the changed values of sceneGraph.modelInstances._instances in ModelInstancesUpdateStage.update) is not stored anywhere, so I thought testing the buffer made sense.

[lukemckinstry]

If this is the case, I believe we should re-run the model pipeline with model.resetDrawCommands();. For an example, see updateClippingPlanes in Model.js. resetDrawCommands is called when the collection changes state.

That works. But it raises some question about API design

• Do we still need ModelInstanceUpdateStage?
• Do we call model.resetDrawCommands every time the user calls ModelInstanceCollection.add or remove. Or do we expose a function like ModelInstanceCollection.update(model) which runs model.resetDrawCommands on the supplied model?

[ggetz]

That works. But it raises some question about API design

@lukemckinstry and I discussed this offline—

We'll still need ModelInstanceUpdateStage to avoid rerunning the entire pipeline when one instance is updated. We'll re-run the pipeline when an instance is added or removed, as we'll need to reallocate the buffers containing the instance data.

Updates can be managed on the ModelInstance instances themselves, setting dirty flags as needed. The update stage should be able to handle those accordingly.

[ggetz]

Is it possible to summarize why this constraint exists?

@javagl I think mostly for API simplicity.

We're accounting for double precision for any instance locations on the globe. So a localized modelMatrix with instances relative to that shouldn't be needed for precision. Do you have another use cases in mind for providing a non-identity model matrix?

[javagl]

The question was unrelated to precision. (Precision is a tricky issue here, but ... unrelated for now).

I rather thought about cases where people want to create instances in a known, local coordinate space. Think about an airport runway where 200 lights are left and right of the runway, along a straight line, 10 meters apart. And then, users want to put these instanced models at a certain position on the globe, and use the modelMatrix for that.

In terms of convenience, the "best" API certainly depends on the use case. For example, in this screenshot, I used lat/lon/height as the input. In other cases, people might only have the local transforms. There may also be cases where the instancing information is given as TRS properties. And people will have to write quite a bit of boilerplate code to assemble these into Matrix4 objects, put them into the array, and maybe squeeze in the ENU-to-FF-matrix for the desired geolocation here.

(API design is often sort of a trade-off ... about shifting the responsibility for doing things (correctly!) between the implementor and the user. I think that the API should be easy to use correctly, and hard to use incorrectly. A seemingly(!) very specific aspect here is: People will create instances. They will set the modelMatrix. They will open an issue because the "model matrix does not work"...)

[javagl]

I've seen that there are a bunch of conflicts. Most of them are in ModelSceneGraph. And they are "my fault", so to speak, caused by a tiny, tiny part of the draping. The buildDrawCommands function does many different things (and it is doing some of them wrong - namely, the computation of the bounding volumes). So I did split this up into...

ModelSceneGraph.prototype.buildDrawCommands = function (frameState) {
  const modelRenderResources = this.buildRenderResources(frameState);
  this.computeBoundingVolumes(modelRenderResources);
  this.createDrawCommands(modelRenderResources, frameState);
};

with computeBoundingVolumes being explicit and saying
NOTE: This contains some bugs. See https://github.com/CesiumGS/cesium/issues/12108

This was only an attempt to follow the "boy scout rule". It was not a change that affected the funcitonality. It was supposed to help isolating and fixing that issue (and maybe even to cover it with specs).

Now... the current state of the ModelSceneGraph in this branch has been changed significantly. (Some of the changes may be caused by attempts to work around that bug, but who knows). The current state contains 13 (!) TODOs. I'm not sure how to handle that. (I'd prefer to keep the split-up function and to not introduce additional flags and properties and bounding volumes, but ... whatever has to be done to get instancing working...). So in doubt, one way of resolving this would be to simply overwrite the current (main) state of the class with the state from this branch.

This is done here: https://github.com/CesiumGS/cesium/tree/model-instance-v2-tests

Maybe I can allocate time for another dedicated cleanup pass for the ModelSceneGraph which replicates the cleanup from the commit linked above. We have to fix that bounding volume issue...

[javagl]

From a quick test, it looks like the current state also works for a model with a non-identity modelMatrix? (I haven't checked the code, only ran a small sandcastle). One open point might be whether model.boundingSphere should take into account the instances. (Currently, it does not seem to do this, but might be useful for flyToBoundingSphere(model.boundingSphere)). Maybe that's already on the radar.

[lukemckinstry]

the current state of the ModelSceneGraph in this branch has been changed significantly. (Some of the changes may

be caused by attempts to work around that bug, but who knows)
These changes were part of the refactor Gabby did to solve the problems we encountered with glTF with multiple hierarchical nodes or glTF with a top-level transform. The transforms in the tree were not being applied correctly, leading to child nodes not aligning to their parent. We need to document this better.

ToDos

These are leftover from the refactor, we will resolve these.

one way of resolving this would be to simply overwrite the current (main) state of the class with the state from this branch.

This looks good. The runtime model instance feature appears to be working correctly & the same as this branch

Maybe I can allocate time for another dedicated cleanup pass for the ModelSceneGraph which replicates the cleanup from the commit linked above. We have to fix that bounding volume issue...

Does the commit fix the bounding volume issues you are referring to. Or is it just an incremental step towards helping isolate and later fix?

it looks like the current state also works for a model with a non-identity modelMatrix

That is good to hear. If you have an example sandcastle can you share the link? I had issues when I briefly tried testing this earlier but ran out of time to verify.

One open point might be whether model.boundingSphere should take into account the instances.

Worth looking into further and discussing.

[javagl]

Does the commit fix the bounding volume issues you are referring to. Or is it just an incremental step towards helping isolate and later fix?

This commit did not change the functionality (i.e. also not fix the issue).

The reason for that change was that buildDrawCommands does some farily unrelated things, namely

• building the render resources
• computing the bounding volumes
• creating the draw commands

(c.f. "cohesive"). So I wanted to break it into buildRenderResources, computeBoundingVolumes, and createDrawCommands (and document what each function is doing).

The fix for the bounding volume computation would then go into ... drumroll ... computeBoundingVolumes 🙂 And I think that the change that is described at #12634 (comment) (a fairly trivial one, after all) could already solve most error cases (and be a general improvement, even if some cases might still be "not perfect").

Once the changes from this PR are in, I'll probably ...

• ...replay the changes from the commit that caused the conflict (because I think that it makes sense to break that down into multiple functions)
• ...add the fix (or at least, improvement) for Wrong bounding volume computations for model #12108 (and all the linked/related issues)

---

That is good to hear. If you have an example sandcastle can you share the link?

I've been doing some tests that had actually been more related to the bounding volume issue, but also to ... all other issues that may be related to "transforms" in one way or another, and I ran some tests with these on the instancing branch as well. These tests included the creation of different flavors of a "unit cube" with a very elaborate texture, to see whether something is right or wrong...

unitCube.zip

However, here is a sandcastle that has a flag useModelMatrix:

• when this is true, then the model.modelMatrix will be set for the geo-placement
• when this is false, then the geo-placement will be "baked" into the instance transforms

The instance transforms are just a grid of size x size x size instances, with the instance at (x, y, z) receives a translation/rotation/scale that is interpolated from a given range, for that grid position. (Again: To see whether something is right or wrong).

I changed the URL to the MilkTruck - it doesn't matter for now, and also looks neat:

const viewer = new Cesium.Viewer("cesiumContainer");

const model = await Cesium.Model.fromGltfAsync({
  //url: "../../SampleData/models/unitCube/unitCube_T_TR.glb",
  url: "../../SampleData/models/CesiumMilkTruck/CesiumMilkTruck.glb",
});
viewer.scene.primitives.add(model);

const useModelMatrix = false;

const geoTransform = Cesium.Transforms.eastNorthUpToFixedFrame(
  Cesium.Cartesian3.fromDegrees(-75.1652, 39.9526, 20)
);

if (useModelMatrix) {
  model.modelMatrix = geoTransform;
}

const size = 5;
const minTranslationX = 0;
const maxTranslationX = 20;
const minTranslationY = 0;
const maxTranslationY = 20;
const minTranslationZ = 0;
const maxTranslationZ = 20;

const minRotationDegX = 0;
const maxRotationDegX = 90;
const minRotationDegY = 0;
const maxRotationDegY = 90;
const minRotationDegZ = 0;
const maxRotationDegZ = 90;

const minScaleX = 1;
const maxScaleX = 2;
const minScaleY = 1;
const maxScaleY = 2;
const minScaleZ = 1;
const maxScaleZ = 2;

function matrixFromAngleDegX(angleDegX) {
  const m = Cesium.Matrix3.fromRotationX(Cesium.Math.toRadians(angleDegX));
  return Cesium.Matrix4.fromRotation(m);
}
function matrixFromAngleDegY(angleDegY) {
  const m = Cesium.Matrix3.fromRotationY(Cesium.Math.toRadians(angleDegY));
  return Cesium.Matrix4.fromRotation(m);
}
function matrixFromAngleDegZ(angleDegZ) {
  const m = Cesium.Matrix3.fromRotationZ(Cesium.Math.toRadians(angleDegZ));
  return Cesium.Matrix4.fromRotation(m);
}

for (let x = 0; x < size; x++) {
  for (let y = 0; y < size; y++) {
    for (let z = 0; z < size; z++) {
      const alphaX = x / (size - 1);
      const alphaY = y / (size - 1);
      const alphaZ = z / (size - 1);

      const translationX =
        minTranslationX + alphaX * (maxTranslationX - minTranslationX);
      const translationY =
        minTranslationY + alphaY * (maxTranslationY - minTranslationY);
      const translationZ =
        minTranslationZ + alphaZ * (maxTranslationZ - minTranslationZ);
      const translation = new Cesium.Cartesian3(
        translationX,
        translationY,
        translationZ
      );
      const translationMatrix = Cesium.Matrix4.fromTranslation(translation);

      const rotationDegX =
        minRotationDegX + alphaX * (maxRotationDegX - minRotationDegX);
      const rotationDegY =
        minRotationDegY + alphaY * (maxRotationDegY - minRotationDegY);
      const rotationDegZ =
        minRotationDegZ + alphaZ * (maxRotationDegZ - minRotationDegZ);
      const rotationMatrix = Cesium.Matrix4.clone(Cesium.Matrix4.IDENTITY);
      Cesium.Matrix4.multiply(
        rotationMatrix,
        matrixFromAngleDegX(rotationDegX),
        rotationMatrix
      );
      Cesium.Matrix4.multiply(
        rotationMatrix,
        matrixFromAngleDegY(rotationDegY),
        rotationMatrix
      );
      Cesium.Matrix4.multiply(
        rotationMatrix,
        matrixFromAngleDegZ(rotationDegZ),
        rotationMatrix
      );

      const scaleX = minScaleX + alphaX * (maxScaleX - minScaleX);
      const scaleY = minScaleY + alphaY * (maxScaleY - minScaleY);
      const scaleZ = minScaleZ + alphaZ * (maxScaleZ - minScaleZ);
      const scale = new Cesium.Cartesian3(scaleX, scaleY, scaleZ);
      const scaleMatrix = Cesium.Matrix4.fromScale(scale);

      let instanceTransform = Cesium.Matrix4.clone(Cesium.Matrix4.IDENTITY);
      Cesium.Matrix4.multiply(
        instanceTransform,
        translationMatrix,
        instanceTransform
      );
      Cesium.Matrix4.multiply(
        instanceTransform,
        rotationMatrix,
        instanceTransform
      );
      Cesium.Matrix4.multiply(
        instanceTransform,
        scaleMatrix,
        instanceTransform
      );

      // When not using the modelMatrix, bake the geoTransform
      // into the instanceTransform
      if (!useModelMatrix) {
        Cesium.Matrix4.multiply(
          geoTransform,
          instanceTransform,
          instanceTransform
        );
      }
      model.instances.add(instanceTransform);
    }
  }
}

model.readyEvent.addEventListener(() => {
  console.log(model.boundingSphere);
  //viewer.scene.camera.flyToBoundingSphere(model.boundingSphere);
});

viewer.scene.camera.setView({
  destination: new Cesium.Cartesian3(
    1253512.5232461668,
    -4732922.214567729,
    4074115.474546098
  ),
  orientation: new Cesium.HeadingPitchRoll(
    2.205737333179613,
    -0.7255022564055849,
    6.283181225638178
  ),
});

Note: The appearance of the instances is different, depending on whether the model matrix is used. This might just be another flavour of #12310 ...

[javagl]

This doesn't seem to be in the repo yet. I'm just mentioning it. It could make sense to add this last, when all other decisions are made, to not bloat the git history with possible additions/removals of large binaries. (BTW: Also make sure to check the model copyright).

[lukemckinstry]

Thanks I agree it makes sense to wait to add the model until the end and we are sure we want to use them.

The model is free to download here https://www.fab.com/listings/3f826cd9-1caf-4a6a-9e8c-42118c1297cf

As detailed in this comment #12588 (comment) I initially used a much larger model https://www.fab.com/listings/c6efa5d8-d94d-4da0-8747-09e0d4af7fb1 but this caused performance issues.

Both models have the Creative Commons Attribution (CC BY 4.0) license, so to my understanding we are free to use the models as long as we provide attribution

[DaviCoder]

I'm also experiencing this issue, and it's quite impactful for my workflow, especially when dealing with multiple selections. I agree with the original request; being able to highlight selected instances, similar to how it worked in earlier versions (e.g., 1.96 with ModelInstanceCollection), would be a significant improvement. I'd love to see this functionality return or be implemented in a new, robust way.

[rudacs]

@lukemckinstry testing with these new commits, I noticed a problem.

If I have several instances, one with the correct position and another, with Matrix4.ZERO.

And when I try to pick, it ends up selecting the wrong instance.
It seems that when the instance is Matrix4.ZERO, it places it in the same position as instance 0.

//modelMatrix = ...
model.instances.add(modelMatrix);
model.instances.add(Matrix4.ZERO);

By removing it from ModelInstancesUpdateStage.js, it works again.

sceneGraph.modelInstances._dirty = false;

[lukemckinstry]

And when I try to pick, it ends up selecting the wrong instance. It seems that when the instance is Matrix4.ZERO, it places it in the same position as instance 0.

Can you explain your reasoning for supplying the zero transform in model.instances.add(Matrix4.ZERO)? The design we had in mind is that each instance would have some non-zero and non-identity transform.

[rudacs]

And when I try to pick, it ends up selecting the wrong instance. It seems that when the instance is Matrix4.ZERO, it places it in the same position as instance 0.

Can you explain your reasoning for supplying the zero transform in model.instances.add(Matrix4.ZERO)? The design we had in mind is that each instance would have some non-zero and non-identity transform.

I've seen better performance from having multiple instances already created and simply repositioning them, rather than creating and removing them every time.

Since I have thousands, I only instantiate what appears on the screen.

[rudacs]

Another problem I encountered is that if you remove and add another item, it doesn't "remove" or "add"
I imagine something in the position update

const viewer = new Cesium.Viewer("cesiumContainer");

let model;
try {
  model = await Cesium.Model.fromGltfAsync({
    url: "../../SampleData/models/wind_turbine.glb"
  });
  viewer.scene.primitives.add(model);
} catch (error) {
  console.log(`Failed to load model. ${error}`);
}

const positions = [
  42.77379, -78.28529,
  42.77549, -78.28439
];

let position;

let instanceTransform;
for (let i = 0; i < positions.length; i += 2) {
  const chunk = positions.slice(i, i + 2);
  position = Cesium.Cartesian3.fromDegrees(chunk[1], chunk[0], 0);
  instanceTransform = new Cesium.Transforms.eastNorthUpToFixedFrame(position);
  model.instances.add(instanceTransform);
}

viewer.scene.camera.setView({
  destination: new Cesium.Cartesian3(
    950971.9555012125,
    -4592451.1790314745,
    4309468.958291063,
  ),
  orientation: new Cesium.HeadingPitchRoll.fromDegrees(80.61, -7.2, 0.0),
});

function handleSelection(screenPosition) {
  const picked = viewer.scene.pick(screenPosition, 15, 15);
  
  if (!picked) {
    return;
  }
  
  const instance = model.instances.get(picked.instanceId);
  model.instances.remove(instance);

  const position = Cesium.Cartesian3.fromDegrees(-78.2828, 42.77989, 0);
  const instanceTransform = new Cesium.Transforms.eastNorthUpToFixedFrame(position);
  model.instances.add(instanceTransform);
}

const handler = new Cesium.ScreenSpaceEventHandler(viewer.camera.canvas);
handler.setInputAction((click) => handleSelection(click.position), Cesium.ScreenSpaceEventType.LEFT_CLICK);

[javagl]

The design we had in mind is that each instance would have some non-zero and non-identity transform.

I don't know why there should be a requirement for "non-identity", but haven't been tracking the development here closely (recently). That may depend on whether the transforms are ~"expected to be in some 'local' space or not", and I assume that this will be pointed out clearly in the documentation.

The point about the 'zero' transform is an interesting and valid one, and likely what rudacs referred to: I think that it is not uncommon to use a 'scale of 0.0' to essentially hide something. And there are several trade-offs, where the question of what is 'good' or 'bad' strongly depends on the usage pattern. Imagine that you want to start showing 0 instances, then incrementally show 1...10000 instances, then incrementally hide them again (think of something like ~"time-dependent construction sites on the globe" or so). In this case, using 'zero' to make an instance invisible is certainly preferable in terms of performance. The alternative would probably be to start with an instances buffer of length 0, and then do 10000 re-allocations (including copies) to expand that, and then another 10000 re-allocations incrementally shrink that buffer. That's one way of keeping the memory bus busy. Anticipating this difference, from an idealistic, longer-term engineering perspective, could lead to the consideration to differentiate beween DynamicInstances and StaticInstances, but ... that's too idealistic.

[lukemckinstry]

Thanks for pointing out the behavior you see @rudacs with regards to picking when an instance transform is set to Matrix4.ZERO.

And thanks as well for pointing out the issue around the ModelInstanceCollection add or remove functions and for proposing a fix. We will review shortly.

using 'zero' to make an instance invisible is certainly preferable in terms of performance. The alternative would probably be to start with an instances buffer of length 0, and then do 10000 re-allocations (including copies) to expand that, and then another 10000 re-allocations incrementally shrink that buffer.

Thanks for making this clear @javagl. The design of the current implementation so far:

• When ModelInstanceCollection add or remove is called, we run resetDrawCommands so everything in the shader is set up again. All the PipelineStages including RuntimeModelInstancingPipelineStage are run again.
• When a single instance is edited, we rebuild the buffer runtimeNode.instancingTransformsBuffer. This is done in ModelInstancingUpdateStage. As pointed out by javagl rebuilding this entire buffer is a larger operation than needed.

So definitely seems possible to optimize design in the manner you point out as possible to better fit this use case of showing/hiding instances by setting scale to 0.

[rudacs]

@lukemckinstry

I was implementing a show inside the ModelInstance, passing a variable to the FS and discarding it.
This would make it more readable that I want to hide that element.

What do you think? Would it be ideal?

I also implemented it in my branch to allow changing the color of the instance.
#12836

[rudacs]

Another problem I encountered is that if you remove and add another item, it doesn't "remove" or "add" I imagine something in the position update

const viewer = new Cesium.Viewer("cesiumContainer");

let model;
try {
  model = await Cesium.Model.fromGltfAsync({
    url: "../../SampleData/models/wind_turbine.glb"
  });
  viewer.scene.primitives.add(model);
} catch (error) {
  console.log(`Failed to load model. ${error}`);
}

const positions = [
  42.77379, -78.28529,
  42.77549, -78.28439
];

let position;

let instanceTransform;
for (let i = 0; i < positions.length; i += 2) {
  const chunk = positions.slice(i, i + 2);
  position = Cesium.Cartesian3.fromDegrees(chunk[1], chunk[0], 0);
  instanceTransform = new Cesium.Transforms.eastNorthUpToFixedFrame(position);
  model.instances.add(instanceTransform);
}

viewer.scene.camera.setView({
  destination: new Cesium.Cartesian3(
    950971.9555012125,
    -4592451.1790314745,
    4309468.958291063,
  ),
  orientation: new Cesium.HeadingPitchRoll.fromDegrees(80.61, -7.2, 0.0),
});

function handleSelection(screenPosition) {
  const picked = viewer.scene.pick(screenPosition, 15, 15);
  
  if (!picked) {
    return;
  }
  
  const instance = model.instances.get(picked.instanceId);
  model.instances.remove(instance);

  const position = Cesium.Cartesian3.fromDegrees(-78.2828, 42.77989, 0);
  const instanceTransform = Cesium.Transforms.eastNorthUpToFixedFrame(position);
  model.instances.add(instanceTransform);
}

const handler = new Cesium.ScreenSpaceEventHandler(viewer.camera.canvas);
handler.setInputAction((click) => handleSelection(click.position), Cesium.ScreenSpaceEventType.LEFT_CLICK);

Changing the position also has no effect.

const viewer = new Cesium.Viewer("cesiumContainer");

let model;
try {
  model = await Cesium.Model.fromGltfAsync({
    url: "../../SampleData/models/wind_turbine.glb"
    //minimumPixelSize: 32,
  });
  viewer.scene.primitives.add(model);
} catch (error) {
  console.log(`Failed to load model. ${error}`);
}

const lng = -75.1652;
const lat = 39.9526;
const positions = [
  42.77789, -78.2837
];

let position;

let instanceTransform;
for (let i = 0; i < positions.length; i += 2) {
  const chunk = positions.slice(i, i + 2);
  position = Cesium.Cartesian3.fromDegrees(chunk[1], chunk[0], 0);
  instanceTransform = new Cesium.Transforms.eastNorthUpToFixedFrame(position);
  const inst = model.instances.add(instanceTransform);
  
  inst.color = i % 3 === 0 ? Cesium.Color.GREEN : Cesium.Color.BLUE;
}

viewer.scene.camera.setView({
  destination: new Cesium.Cartesian3(
    950971.9555012125,
    -4592451.1790314745,
    4309468.958291063,
  ),
  orientation: new Cesium.HeadingPitchRoll.fromDegrees(80.61, -7.2, 0.0),
});

function handleSelection(screenPosition) {
  const instance = model.instances.get(0);

  const position = Cesium.Cartesian3.fromDegrees(-78.2828, 42.77989, 0);
  const instanceTransform = Cesium.Transforms.eastNorthUpToFixedFrame(position);
  
  instance.position = instanceTransform;
}

const handler = new Cesium.ScreenSpaceEventHandler(viewer.camera.canvas);
handler.setInputAction((click) => handleSelection(click.position), Cesium.ScreenSpaceEventType.LEFT_CLICK);

This partially solved it, but I realized that it should actually be adding and removing instances, which would be the same amount. So it would be like updating the position; the vertex should pass through. But if I keep it here, it doesn't work.

ModelInstancesUpdateStage.update...
sceneGraph.modelInstances._dirty = false;

I'm trying to see the solution for this.
I'm trying to find a solution for this, but so far I've had no luck.