Added Entity.trackingReferenceFrame #12194

jfayot · 2024-09-15T13:23:04Z

Description

Added Entity.trackingReferenceFrame to allow tracking entities in their own reference frame.

Issue number and link:

Fixes #12293

Testing plan

Updated:

EntitySpec.js
EntityViewerSpec.js
to cover this new property

Updated Sandcastles CallbackPositionProperty and Interpolation for demo

Tested in the 3 different scene modes.
Tested non regression on fast satellites and slow moving objects.

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 · 2024-09-15T13:23:24Z

Thank you for the pull request, @jfayot!

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

ggetz · 2024-09-16T18:57:03Z

Thanks for the PR @jfayot! Would you mind expanding a bit on the motivation behind this change? Is it mostly to address #11657?

jfayot · 2024-09-16T19:35:35Z

Not really, in fact. I wanted to relate this PR to something similar, but the real motivation is to achieve the same effect as what you can see in Callback Position Property sandcastle in a one-liner and still be able to move the camera around the tracked entity.
May be the best would be to try the updated Interpolation sandcastle to give you an idea...

ggetz · 2024-09-18T13:58:12Z

@jfayot Thank you, got it. I do think your Sandcastle example is useful behavior!

My concern here is I think there is some mismatch about how CesiumJS defines an "inertial reference frame". It appears that you are using it to refer to the entity's inertial reference frame (as in this example). In the rest of the CesiumJS API, I believe it is used to refer to Earth's inertial reference frame, or ECI. What are your thoughts?

jfayot · 2024-09-18T14:13:42Z

You're perfectly right: the ReferenceFrame.INERTIAL refers to the International Celestial Reference Frame, not to the entity's inertial reference frame.
Would it be ok if I rename the ReferenceFrame enum like this ?

EARTH_FIXED
CELESTIAL
BODY (or ENTITY)

jfayot · 2024-09-18T18:12:34Z

Or may be just create a new enum:

/**
 * Constants for identifying well-known tracking reference frames.
 *
 * @enum {number}
 */
const TrackingReferenceFrame = {
  /**
   * The east-north-up entity's frame.
   *
   * @type {number}
   * @constant
   */
  EAST_NORTH_UP: 0,

  /**
   * The entity's inertial frame.
   *
   * @type {number}
   * @constant
   */
  INERTIAL: 1,
};

ggetz · 2024-09-20T14:15:17Z

Or may be just create a new enum:

I do think it's best to separate this behavior from the position's ReferenceFrame. 👍

@jfayot My only remaining concern is that we do have some built-in behavior where EntityView.js "autodetects" satellites and and other orbital entities based on how fast they are moving and changes the camera view to Earth's inertial reference frame. I'm concerned that specifying EAST_NORTH_UP or INERTIAL will be confusing if that autodetect kicks in. What are your thoughts here?

We'll also need to test to ensure everything is working with the satellite case before this PR is merged.

jfayot · 2024-09-20T15:16:38Z

Got it @ggetz !
You're right: EAST_NORTH_UP should be renamed AUTO_DETECT with a clear documentation of its behavior.
Anyhow, my implementation of inertial tracking overrides this auto-detect behavior. Is that the good way to go, or should auto-detect always happen even if INERTIAL is chosen?
IMO it should not: I've tested INERTIAL on satellites, and it's not that bad: there is a very little cycling shift that comes over time, but nothing compared to east-north-up tracking mode (I can't even say if this shift comes from the entity's orientation or not, but if it's the case, then this is what is expected in this tracking mode).
I'll update the PR to make things clearer! I'll probably also update the Sandcastle demo to be able to test satellites.
And one last thing if you think it has any value: I'll add another tracking option: INERTIAL_WITHOUT_ROLL (any other name welcomed)... wait for the demo!

jfayot · 2024-09-24T09:45:00Z

@ggetz I have added a new sandcastle demo to illustrate the different tracking reference frames: Entity tracking.

jfayot · 2024-10-07T12:52:21Z

Hi @ggetz ! Any update on this PR?

ggetz · 2024-10-07T13:27:58Z

Thank you @jfayot! We'll get back to this in the next few days. Thanks for your patience, we appreciate your contributions!

ggetz

Awesome, I think this is a good approach @jfayot.

Apologies for the delay in getting back to this PR! I hope you still have interest in getting this change merged.

ggetz · 2024-10-29T13:42:09Z

packages/engine/Source/Core/TrackingReferenceFrame.js

+   * @type {number}
+   * @constant
+   */
+  INERTIAL: 1,


Would it make sense to fall back to autodetect in the case where an entity has no defined orientation?

I agree on that point, as we already have TrackingReferenceFrame.VELOCITY. I'll change that!

ggetz · 2024-10-29T13:44:04Z

packages/engine/Source/Core/TrackingReferenceFrame.js

+   * @type {number}
+   * @constant
+   */
+  VELOCITY: 2,


What happens when an entity's velocity is 0?

This is mentioned in #8900: The issue is that if the vehicle stops, the orientation becomes undefined, causing an abrupt change in the vehicle's orientation
The behavior will be the same here, causing the camera position to change abruptly following the entity's orientation

ggetz · 2024-10-29T13:46:09Z

packages/engine/Specs/DataSources/EntityViewSpec.js

+      const entity = new Entity();
+      entity.trackingReferenceFrame = TrackingReferenceFrame.INERTIAL;
+      const view = new EntityView(entity, scene);
+      view.update(JulianDate.now());


Should there be an expect assertion in this test?

In fact this test is no more needed as the case descibed here has the AUTODETECT fallback...

ggetz · 2024-10-29T13:49:47Z

packages/engine/Source/Core/TrackingReferenceFrame.js

+   * @type {number}
+   * @constant
+   */
+  AUTODETECT: 0,


Many of our enums use NONE for an undefined or default state. Does that make sense here>

Most of the enum with a NONE value I've seen are reflecting something meaningful:

ArcType.NONE

ExtrapolationType.NONE

Visibility.NONE

HeightReference.NONE

...
I'm not sure a TrackingReferenceFrame.NONE value would be of any interest. This would represent an invalid value that should be tested against, and add more noise to the code IMO.
Unless you would prefer me to replace the TrackingReferenceFrame.AUTODETECT by TrackingReferenceFrame.NONE?

jfayot · 2024-10-29T16:34:57Z

Hi @ggetz! Thanks for your comments. This is ready for another review.

…in their own inertial reference frame

…an orientation defined

…esiumGS#9159 and CesiumGS#8900)

…fused with ReferenceFrame ones

… AUTODETECT

ggetz · 2024-11-15T18:38:33Z

Looking good @jfayot! I pushed some minor formatting tweaks to the documentation, but otherwise this is great!

Thanks!

ggetz added the PR - Changes Requested label Sep 18, 2024

ggetz self-assigned this Sep 20, 2024

ggetz requested changes Oct 29, 2024

View reviewed changes

jfayot force-pushed the feat_tracking_reference_frame branch from 4cae947 to 80c2e14 Compare October 29, 2024 16:24

ggetz mentioned this pull request Nov 7, 2024

Camera orientation problem with sampledPositionProperty and trackedEntity #12293

Closed

jfayot added 8 commits November 9, 2024 18:05

feat: added Entity.trackingReferenceFrame to allow tracking entities …

76cd99c

…in their own inertial reference frame

fix: only use VelocityOrientationProperty if the entity doesn't have …

916689c

…an orientation defined

fix: not ready for defaulting to VelocityOrientationProperty yet (see C…

295d6fb

…esiumGS#9159 and CesiumGS#8900)

fix: created dedicated TrackingReferenceFrame constants not to be con…

78cc807

…fused with ReferenceFrame ones

doc: added entity tracking sandcastle demo

917adb2

fix: INERTIAL tracking without orientation property now falls back to…

4b3303d

… AUTODETECT

test: removed obsolete test case

32c8e63

fix: fixed prettier

e69487a

jfayot force-pushed the feat_tracking_reference_frame branch from 9d2b51a to e69487a Compare November 9, 2024 17:37

ggetz removed the PR - Changes Requested label Nov 15, 2024

ggetz added 2 commits November 15, 2024 11:22

Merge branch 'main' into feat_tracking_reference_frame

3a019bb

Documentation tweaks

d9e3859

ggetz approved these changes Nov 15, 2024

View reviewed changes

ggetz added this pull request to the merge queue Nov 15, 2024

Merged via the queue into CesiumGS:main with commit 09a0276 Nov 15, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Entity.trackingReferenceFrame #12194

Added Entity.trackingReferenceFrame #12194

jfayot commented Sep 15, 2024 •

edited

Loading

github-actions bot commented Sep 15, 2024

ggetz commented Sep 16, 2024

jfayot commented Sep 16, 2024

ggetz commented Sep 18, 2024

jfayot commented Sep 18, 2024

jfayot commented Sep 18, 2024

ggetz commented Sep 20, 2024

jfayot commented Sep 20, 2024

jfayot commented Sep 24, 2024

jfayot commented Oct 7, 2024

ggetz commented Oct 7, 2024

ggetz left a comment

ggetz Oct 29, 2024

jfayot Oct 29, 2024

ggetz Oct 29, 2024

jfayot Oct 29, 2024

ggetz Oct 29, 2024

jfayot Oct 29, 2024

ggetz Oct 29, 2024

jfayot Oct 29, 2024

jfayot commented Oct 29, 2024

ggetz commented Nov 15, 2024

Added Entity.trackingReferenceFrame #12194

Added Entity.trackingReferenceFrame #12194

Conversation

jfayot commented Sep 15, 2024 • edited Loading

Description

Issue number and link:

Testing plan

Author checklist

github-actions bot commented Sep 15, 2024

ggetz commented Sep 16, 2024

jfayot commented Sep 16, 2024

ggetz commented Sep 18, 2024

jfayot commented Sep 18, 2024

jfayot commented Sep 18, 2024

ggetz commented Sep 20, 2024

jfayot commented Sep 20, 2024

jfayot commented Sep 24, 2024

jfayot commented Oct 7, 2024

ggetz commented Oct 7, 2024

ggetz left a 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

Choose a reason for hiding this comment

jfayot commented Oct 29, 2024

ggetz commented Nov 15, 2024

jfayot commented Sep 15, 2024 •

edited

Loading