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:

• [x] broken tests (mostly classes impacted by the scene graph transforms refactor)
  • [x] ModelSceneGraph
  • [x] ModelDrawCommands
• [x] Add tests
  • [x] ModelInstance Class
  • [x] RuntimeModelInstancingPipelineStage
  • [x] 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)
  • [x] Cannot supply instances to a model with glTF with ext_mesh_gpu_instance extension, throw developer error
  • [ ] 2D and columbus view - https://github.com/CesiumGS/cesium/pull/12816
• [x] 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 https://github.com/CesiumGS/cesium/pull/12744
• Map views
  • 2D
  • Columbus View
• Review regression other sandcastles
  • 3D Models

Author checklist

• [x] I have submitted a Contributor License Agreement
• [x] 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[bot]]

Thank you for the pull request, @lukemckinstry!

:white_check_mark: 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.

[lukemckinstry]

Another limitation that I should have included in the above comment but was not top of mind was that the current runtime model instancing implementation does not support changing the size of the instances buffer ie. number of instances arbitrarily over time. We support changing the instances themselves, but not the number available.

I believe we talked about this being OK for initial release earlier in development , but implementing the collection (ModelInstanceCollection with add and remove functions) this afternoon made this limitation a little more apparent. I do see how we can provide clear instructions so developers understand (initial) limitation.

[ggetz]

I believe we talked about this being OK for initial release earlier in development , but implementing the collection (ModelInstanceCollection with add and remove functions) this afternoon made this limitation a little more apparent. I do see how we can provide clear instructions so developers understand (initial) limitation.

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.

[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.

[javagl]

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

Is it possible to summarize why this constraint exists? (Beyond: "Well, all these matrices... it's complicated...")

One could argue that it's possible to "emulate" any modelMatrix, by baking this modelMatrix into the instances matrices. But that's also why that constraint could be hard to justify.

[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.

[lukemckinstry]

Added a new Sandcastle 3D Model Instancing Terrain.html in this commit https://github.com/CesiumGS/cesium/pull/12588/commits/95e92f3c4c9e7930f54021d72797138893cf506a

But the performance is slower than expected with roughly 1400 instances. On the initial sandcastle used for development 3D Model Instancing.html performance was strong with up to 10000 instances (in a more contained area, there may be other factors to help explain too).

Dropping this note here as a reminder to investigate and perform benchmarking.

[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 https://github.com/CesiumGS/cesium/issues/12634#issuecomment-2924994801 (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 https://github.com/CesiumGS/cesium/issues/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 https://github.com/CesiumGS/cesium/issues/12310 ...

[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.

[lukemckinstry]

Added a new Sandcastle 3D Model Instancing Terrain.html in this commit 95e92f3

But the performance is slower than expected with roughly 1400 instances. On the initial sandcastle used for development 3D Model Instancing.html performance was strong with up to 10000 instances (in a more contained area, there may be other factors to help explain too).

Dropping this note here as a reminder to investigate and perform benchmarking.

The original wind turbin glTF I used https://www.fab.com/listings/c6efa5d8-d94d-4da0-8747-09e0d4af7fb1 is 14MB, which is 10-20x bigger than other models we use in SampleData.

So I swapped in this wind turbin https://www.fab.com/listings/3f826cd9-1caf-4a6a-9e8c-42118c1297cf that is 4MB (still bigger than most of the SampleData) and there is no observable lag for the sandcastle.

The sandcastle demo plots 1400 instances over a wide area (state of New York). So it seems like this is acceptable performance for this feature, but I'll continue more benchmarking to understand roughly where the performance limits are.

[lukemckinstry]

Now... the current state of the ModelSceneGraph in this branch has been changed significantly. (Some ... 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...

I rebased and resolved the merge conflicts between this branch and main in a way to preserve @javagl's refactor of ModelSceneGraph.buildDrawCommands. The refactor definitely made things more clear!

[javagl]

There still seem to be merge conflicts in ModelSceneGraph. Back in https://github.com/CesiumGS/cesium/pull/12588#issuecomment-2945211831 , I mentioned that just "ignoring" my changes could be OK (they hadn't been "functional" changes, but only cleanups, to prepare the stage for finally tackling https://github.com/CesiumGS/cesium/issues/12108). However, I'll revisit this when the dust here has settled.

[lukemckinstry]

ready for review @ggetz

[lukemckinstry]

2D and Columbus support

• 2D - My hunch is this requires setting modelView correctly in the shader
• Columbus - we don't support for i3dm, so I don't think we should support for runtime model instancing since we dont have that working template to start from

[lukemckinstry]

2D and Columbus support

This sandcastle link provides code to check the bounding spheres and they look correct to me in SCENE2D.

One note, SCENE3D and SCENE2D use ModelRuntimePrimitive.boundingSphere. ModelRuntimePrimitive.boundingSphere2D is only used when Model.ProjectTo2D is true.

[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.