Fixes issue #12465 (Entity Tracking broken)

[jfayot]

Description

This is an attempt to fix broken entity tracking when viewFrom is set:
Restored boundingSphereScratch instead of entityView.boundingSphere inside CesiumWidget._onTick.

Issue number and link

Fixes #12465

Testing plan

• Updated EntityViewSpec
• Tested Entity tracking sandcastle
• Tested Clamp Entities to Ground
• Tested this sandcastle which demonstrates the error when setting the viewer.selectedEntity
• Tested this sandcastle to test the original issue of a "flickering" entity when it moves (it should not flicker)

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, @jfayot!

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

[ggetz]

Thank you for the fix @jfayot!

@jjspace Could you please review?

[jjspace]

Can you explain why this was removed? More than it just works now? The correlation between this and the scratch value and the entities actual bounding sphere all still feels a little funky to me.

If we do still want to remove this we will need to go through the deprecation process as this is part of the public API and people may be relying on it.

[jfayot]

I have no answer for you @jjspace.

[jfayot]

@jjspace I recognize that I have no idea why "it just works now!"

Even this bounding sphere parameter inside EntityView.update looks weird: why would you offer this as a parameter when there is no way to get a bounding sphere from an Entity in the public API?

In #3954, the bounding sphere was moved from EntityView constructor to update method, but again, why would you impose to provide a bounding sphere to EntityView (which is related to the high level Entity API) when you don't have the ability to get this bounding sphere from Entity's high level API?

Before #3954, the bounding sphere parameter in the constructor was documented as:
@param {BoundingSphere} [boundingSphere] An initial bounding sphere for setting the default view.

This "initial" aspect has completely disappeared. My guess is that it should be restored, AND that the Entity's bounding sphere should be computed from inside the EntityView implementation and should disappear from the public API.

Otherwise provide a public Entity.computeBoundingSphere method...

[jjspace]

@jfayot I'm sorry this took so long to get back to. It turned out to be a bit of a "land mine" in the code for me to try (read: struggle) to wrap my head around...

I did some digging into this to try and understand it more completely. First I'll say this whole sequence of functions is a total mess imo with way to many "hard to follow" side effects. Ideally it'd be nice to do a refactor pass on all this camera tracking logic but that's way to big for this PR right now (the camera system itself needs some love too)

^{Sorry this turned into a wall of text, I tried to condense but it's important context that I think needs written down somewhere}

The first important thing to realize is that EntityView.update() behaves differently the first/second time it's called vs the subsequent calls that happen every frame. updateTransform is the same way because of the updateLookAt and saveCamera arguments that are potentially only different the "first" time when the objectChanged. _{Side note, the way we're currently using objectChanged will never be true after the first time because we always regenerate the EntityView completely instead of modifying it}

The EntityView.boundingSphere is only set the first time EntityView.update() is called (objectChanged) and the entity does not have a viewFrom property. This is what was causing the satellite tracking to fail because that has a viewFrom in the sandcastle so the EntityView.boundingSpehere is never set. This caused my previous "fix" to CesiumWidget._onTick (in #12429) to never run the update calls because defined(entityView.boundingSphere) was false. I added this check in the first place because DataSourceDisplay.getBoundingSphere requires the bounding sphere to be defined.

If EntityView.boundingSphere is defined then it's center will be used in the updateTranform function instead of the other transform logic. This is why it was important to call dataSourceDisplay.getBoundingSphere with the exisiting entityView.boundingSphere because that function updates that bounding sphere itself which is the same reference inside the EntityView. This wasn't a problem before #124929 because they were all using the same scratch variable in CesiumWidget (in _onTick and updateTrackedEntity) so it was always referring to the same object.

Given all that, an alternative fix for this that works for all the provided examples without changing the EntityView could be this:

const boundingSphereScratch = new BoundingSphere();

/**
 * @private
 */
CesiumWidget.prototype._onTick = function (clock) {
  const time = clock.currentTime;

  const isUpdated = this._dataSourceDisplay.update(time);
  if (this._allowDataSourcesToSuspendAnimation) {
    this._canAnimateUpdateCallback(isUpdated);
  }

  const entityView = this._entityView;
  if (defined(entityView)) {
    const trackedEntity = this._trackedEntity;
    const trackedState = this._dataSourceDisplay.getBoundingSphere(
      trackedEntity,
      false,
      entityView.boundingSphere ?? boundingSphereScratch,
    );
    if (trackedState === BoundingSphereState.DONE) {
      entityView.update(time, undefined);
    }
  }
};

This is a more minimal code change. However, in some ways this feels pretty bad too and requires understanding all the interactions in how we're calling update and it's internal logic that I outlined above.

I think removing EntityView.boundingSphere as you've done could be the right move. This would remove one of the "side effect-ish" behaviors in updateTransform relying on that.boundingSphere. This will (slightly) solidify the logic that EntityView.update sets the initial location of the camera then subsequent calls to updateTransform update the camera relative to how the entity has moved regardless of the bounding sphere.
If this is the approach we take it probably also makes sense to replace the call to getBoundingSphere to a new function that only returns the status for CesiumWidget._onTick since that's all we really care about. Or maybe just make the result optional.
If this makes the most sense to you too please add the deprecation message to the removed property and update getBoundingSphere as/if needed. While it's being deprecated we might need to keep setting it even if we're not using it internally so the scratch variables still need to be updated as required.

Alternatively we could keep EntityView.boundingSphere and make sure it's position is always used even for viewFrom entities but that seems like a bigger change that may not be necessary.

PS: this is one of my top priorities to hopefully still get it in for the release on Monday. I'll be able to take a look over the weekend too to try and keep it moving without more delays on my part

[jfayot]

Thanks for the analysis @jjspace !

If this is the approach we take it probably also makes sense to replace the call to getBoundingSphere to a new function that only returns the status for CesiumWidget._onTick since that's all we really care about. Or maybe just make the result optional.

I'm not sure to understand this: is it the call this._dataSourceDisplay.getBoundingSphere inside CesiumWidget.prototype._onTick that you want to change? If so, then I don't get the idea, because in my understanding, EntityView.prototype.update still needs an initial bounding sphere in order to make the call camera.viewBoundingSphere work properly...

[jjspace]

I'm not sure to understand this: is it the call this._dataSourceDisplay.getBoundingSphere inside CesiumWidget.prototype._onTick that you want to change?

@jfayot Yes that's where I meant. We really only care about the return value from that function. We need it to update the passed in boundingSphere only if we're still using that bounding sphere in the EntityView#updateTransform function to set the cartesian

in my understanding, EntityView.prototype.update still needs an initial bounding sphere in order to make the call camera.viewBoundingSphere work properly...

This is one of the things that makes this whole sequence of code so confusing. If you actually follow the code through the way it's run the call to camera.viewBoundingSphere only happens when objectChanged (functionally only the "first" time) and the entity does not have a viewFrom property. It's only called in one of multiple code paths and is never called after the first time. You can prove this by adding a log statement in camera.viewBoundingSphere and loading the sample sandcastles to check how many times it's called. Also the boundingSphere argument is already marked as optional so it's not required

[jfayot]

Also the boundingSphere argument is already marked as optional so it's not required

Yes you're right, the bounding sphere parameter doesn't change anything!
So if we push the reasoning ahead, the whole code block inside if (!hasViewFrom && defined(boundingSphere)) statement can be removed...
And if so, the saveCamera variable is always true and can also be removed...

Am I getting it right?

[jjspace]

So if we push the reasoning ahead, the whole code block inside if (!hasViewFrom && defined(boundingSphere)) statement can be removed...

@jfayot This code still does something when the boundingSphere is provided and there is not a viewFrom property so I'm not currently confident in removing it completely without more testing that it's not actually needed.
Note that there is a call to update inside updateTrackedEntity at the bottom of CesiumWidget and this is usually the "first" call to the function. The one that sets up everything inside those if statements. AFAIK right now this is the only time objectChanged is true because it's going from undefined to whatever the entity passed into the constructor the line above it is. After that call (triggered by CesiumWidget._postRender after the trackedEntity changes) subsequent calls for the same tracked entity happen inside the _onTick.

[jfayot]

If this is the approach we take it probably also makes sense to replace the call to getBoundingSphere to a new function that only returns the status for CesiumWidget._onTick since that's all we really care about. Or maybe just make the result optional.

@jjspace I get your point now ! However, while testing the different code path, I'm convinced that the call to getBoundingSphere is useless inside _onTick, which could be rewritten like this:

CesiumWidget.prototype._onTick = function (clock) {
  const time = clock.currentTime;

  const isUpdated = this._dataSourceDisplay.update(time);
  if (this._allowDataSourcesToSuspendAnimation) {
    this._canAnimateUpdateCallback(isUpdated);
  }

  const entityView = this._entityView;
  if (defined(entityView)) {
    entityView.update(time);
  }
};

While, off course, keeping the bounding sphere call and parameter in function updateTrackedEntity:

  const bs =
    state !== BoundingSphereState.FAILED
      ? trackedEntityBoundingSphereScratch
      : undefined;
  widget._entityView = new EntityView(trackedEntity, scene, scene.ellipsoid);
  widget._entityView.update(currentTime, bs);

Concerning the deprecation of EntityView.boundingSphere, two questions:

1. in which release would you like to see it completely removed?
2. Although it is in the public API, it is not visible in EntityView's documentation. Should I just add a deprecation note in the header code which will not be visible unless people dig into the sources:

  /**
   * The bounding sphere of the object.
   * @type {BoundingSphere}
   * @deprecated This property has been deprecated and will be removed in Cesium 1.xxx
   */
  this.boundingSphere = undefined;

... or should I make move this to Object.defineProperties(EntityView in order to make its documentation visible?

Object.defineProperties(EntityView, {
  /**
   * The bounding sphere of the object.
   * @type {BoundingSphere}
   * @deprecated This property has been deprecated and will be removed in Cesium 1.xxx
   */
  boundingSphere: {
    get: function () {
      deprecationWarning(
        "EntityView.boundingSphere",
        "EntityView.boundingSphere has been deprecated and will be removed in Cesium 1.xxx",
      );
      return this._boundingSphere;
    },
    set: function (boundingSphere) {
      deprecationWarning(
        "EntityView.boundingSphere",
        "EntityView.boundingSphere has been deprecated and will be removed in Cesium 1.xxx",
      );
      this._boundingSphere = boundingSphere;
    },
  },
});

[jjspace]

I'm convinced that the call to getBoundingSphere is useless inside _onTick

@jfayot I think it might still be needed simply to make the check against the BoundingSphereState.DONE to make sure the entity is actually loaded and ready? if not then I agree this can probably be simplified down. Maybe even as far as the single line version this.entityView?.update(time)?

Concerning the deprecation of EntityView.boundingSphere, two questions:

1. Let's do 3 releases. The release on monday will be 1.127 so 1.130 for this one
2. Just to note, it is in the docs for EntityView. But the second example you posted is better, convert this to a getter/setter and display the deprecation message there. All that matters is that from the perspective of people consuming this API it appears to work the same even if the implementation changed. Also with that change I would make sure to change the place it's currently being set to use the private _boundingSphere property so we're not throwing the warning unless people are actually interacting with it themselves.

Please also remember to update the specs to cover these use cases so we can remain confident the tracking is working as expected in all cases

[jjspace]

@jfayot is that the local docs on this branch where you already removed the property? If so then that's why it's not there! Check the hosted version or locally in main. Running npm run build-docs will rebuild the local docs.

[jfayot]

@jfayot is that the local docs on this branch where you already removed the property? If so then that's why it's not there! Check the hosted version or locally in main. Running npm run build-docs will rebuild the local docs.

Just a wrong jsdoc implementation on my side... sorry for that!

[jfayot]

@jjspace this should be ready for another review pass (although still the same test failures)

[jfayot]

@jjspace when you'll have the time, could you also answer my questions from #11674 please? Thanks in advance!

[jjspace]

Thanks for the changes @jfayot, I think these look good now. I can confirm all 3 of the test sandcastle's we've seen issues with are still working as expected and I tested a handful of other tracked entity instances as well.

@ggetz would you be able to do some testing on this as well? Specifically around any other entity tracking cases you can think of that aren't in those sandcastles. I think the code is good but would like a second pass on actually running it so we can be sure we didn't introduce another bug that I'm not spotting.

[ggetz]

My one concern here is that this would create a new object even when it's not needed, which then needs to get garbage collected later. That's generally fine if the function is not called too frequently, but I think this could be called once every frame, right?

Would it make sense to have another new function getBoundingSphereState which can be used without a result parameter, which getBoundingSphere then uses under the hood?

[jfayot]

@ggetz, @jjspace I'm not sure to clearly understand your concern: if no result is passed to DataSourceDisplay.getBoundingSphere then the test by the end of the function should handle this correctly?

  ...
  boundingSpheres.length = count;
  if (defined(result)) {
    BoundingSphere.fromBoundingSpheres(boundingSpheres, result);
  }
  return BoundingSphereState.DONE;
};

What am I missing?

[ggetz]

Ah, I missed that. Because our library has the convention that when a result is undefined, a new object is created, I assumed this did the same.

Perhaps only to avoid assumptions like mine, we should still create a new function that matches our convention then.

[jfayot]

@ggetz I understand what you mean.

However, optimizing ONLY the returned value wouldn't be of great benefit IMO.
If you look at the function, you'll see that it loops over the vizualizers of the data source and call another getBoundingSphere on each one.
If you would really like to optimize and save created objects and GC, then this would require you to create getBoundingSphereState methods for each vizualizer...
Then some vizualizers themselves rely on updaters to compute their bounding spheres... so again you have to create getBoundingSphereState on updaters...

I'm not sure all these efforts (and associated regression risk) are worth this PR.

However, in order to match the convention that when a result is undefined, a new object is created, I have duplicated code inside getBoundingSphere to create getBoundingSphereState as suggested.
As you can see, we're still creating objects in this function because of the vizualizers. And I was not able to make getBoundingSphere rely on getBoundingSphereState without breaking too much existing code.

CC @jjspace

[ggetz]

I'm not sure all these efforts (and associated regression risk) are worth this PR.

Thanks @jfayot, I agree. I only wanted to make sure we were being careful about new code added.

[ggetz]

The code here, and all of the testing plan looks great to me.

I did notice some trouble tracking an entity which is clamped to ground / 3D Tiles on initial load in the Clamp Entities to Ground Sandcastle. It loads so that the camera is underground for me. Does anyone else see the same?

[ggetz]

If I'm the only one seeing that problem for whatever reason, I can open a separate issue and we can merge this!

[jfayot]

@ggetz I can see it too...

[jjspace]

@ggetz I see that problem on this branch but not main. I was pretty sure I checked that sandcastle last week and it was fine though...

[lukemckinstry]

I see it too, I was not set up correctly before

[jfayot]

@jjspace I know you won't like it, but it looks like this EntityView.boundingSphere is still necessary : I have a fix, but I can't explain why "it just works now"...

[jfayot]

@ggetz, @jjspace roughly, the fix consists in reintroducing the boundingSphere... should I go ahead (that is, removing all the deprecation parts)? Or are you following another track?

[ggetz]

I know you won't like it, but it looks like this EntityView.boundingSphere is still necessary : I have a fix, but I can't explain why "it just works now"...

#12495 changed handling asynchronous updates for billboards and labels. Perhaps it "fixed" something related to timing here?

[ggetz]

Given there is still some uncertainty here, let's hold off on getting this into the release today. Let's make sure we're fixing this once and for all. 👍

[jjspace]

@ggetz @jfayot I believe the issue is that updateTransform starts with a cartesian set to the value of the entity's position at a given time. But for stationary entities that are clamped this value never actually changes so the rest of the updateTransform logic always returns the same location. However the clamping logic does change the location of an entity's boundingSphere. The previous logic which used the EntityView.boundingSphere.center and called DataSourceDisplay.getBoundingSphere to update said value would account for this to ensure we are always looking at the most up to date bounding sphere even if it was modified through other processes

If you switch to main and look closely the camera starts on the label then jumps around with the label as the G3DT gets loaded in and the clamping logic can update it's position

I think this means we can't deprecate the boundingSphere and should keep it around for now. I think the final code will look closer to the more minimal change I outlined in an earlier comment #12467 (comment)

[jfayot]

#12495 changed handling asynchronous updates for billboards and labels. Perhaps it "fixed" something related to timing here?

@ggetz I've tested this branch rebased on top of 1.126 (so without #12495) and I still can see the camera going underground.

I do agree with your analysis @jjspace, we can clearly see the camera jumping quickly to the new entity's bounding sphere.

[jjspace]

Thanks for the changes @jfayot, sorry this couldn't make it out with the release yesterday. I can confirm the 3 previous sandcastle examples and the clamping one are all now working correctly as expected so I think this is probably working how we want it now 🤞

Just had one comment about the specs.
Also @ggetz would you be able to re-test as well just to double check?

[jjspace]

Can you add a test that modifies the bounding sphere to prove that the camera will follow it on future calls to update?

(I didn't check all the other specs so if it already exists just lmk)

[jfayot]

@jjspace, done! This should be ready for another review

[jjspace]

Thanks for the changes @jfayot. One more small comment and I think this can get merged.

@ggetz would you also be able to take one last runthrough of tests just to make sure we're not missing anything?

[jjspace]

This test is valuable to make sure changing EntityView.boundingSphere works but doesn't mimic the behavior of changing the actual bounding sphere it was set up with originally like calling getBoundingSphere does. Maybe another that replaces this line with bs1.center = new Cartesian3(3, 4, 5); would also be valuable.

[jfayot]

Good catch @jjspace. Added new test. Should be ready for a final (hopefully 😅) review!