CesiumGS / cesium

An open-source JavaScript library for world-class 3D globes and maps :earth_americas:
https://cesium.com/cesiumjs/
Apache License 2.0
12.96k stars 3.49k forks source link

CallbackProperty and positions #8944

Closed mramato closed 1 week ago

mramato commented 4 years ago

We've has several users state that they are using CallbackProperty for entity.position. This is not officially support and only works "by accident" if you are always specifying ECF coordinates and not using path visualization.

However, it is not possible to do this in TypeScript because the types do not match up. We should probably just add a CallbackPositionProperty to handle this use case officially and call it a day.

samherrmann commented 4 years ago

I'm not sure if this should be a separate issue, but based on the sandcastle example, line 43, it should be possible to assign a Cartesian3 position to entity.position. This does not currently appear to be the case. Entity.position is typed as PositionProperty, which in the TypeScript definition is incompatible with Cartesian3.

mramato commented 4 years ago

@samherrmann TypeScript does not allow multiple types for properties, so while Cesium via JS allows you to use Cartesian3 and other primitive types, TS does not. You can read through this comment for the details: https://github.com/CesiumGS/cesium/issues/8898#issuecomment-637579223

Instead, you need to use entity.position = new ConstantPositionProperty(cartesian3).

We plan to eventually make TS expose Property types as generics, but haven't had time to work on it yet.

benwiles1 commented 4 years ago

Couldn't you use union types? eg position: Cartesian3 | PositionProperty not sure what that would look like in jsdoc.

samherrmann commented 4 years ago

@benwiles1 I was thinking that too at first, but the problem with that is that when getting a value it would be typed as Cartesian3 | PositionProperty while in reality is always of type PositionProperty. Consequently, to make TypeScript happy, you'd have to do a type check before being able to access the PositionProperty value. So personally I don't think that approach would improve on the current situation where you have to wrap the value in a ConstantPositionProperty when setting it.

Edit

To put my words above into examples...

Type of entity.position is PositionProperty:

// set position:
entity.position = new ConstantPositionProperty(myCartesian3);
// get position:
entity.position.getValue(...);

VS

Type of entity.position is Cartesian3 | PositionProperty:

// set position:
entity.position = myCartesian3;
// get position:
if (entity.position instanceof PositionProperty) {
  entity.position.getValue(...);
}
thw0rted commented 3 years ago

Good news re: the Typescript discussion. It looks like https://github.com/microsoft/TypeScript/pull/42425 is finally on the way, so it'll be possible to type all of these as get: PositionProperty, set: Cartesian3 | PositionProperty.

maxizrinUX commented 2 years ago

There is a simple workaround for this:

// Create new Entity, with no position.
const entity = new CesiumEntity({billboard: img});
// Add the position callback.
(entity as any)["position"] = new Cesium.CallbackProperty(() => cartesian, false);
thw0rted commented 2 years ago

If you're going to anycast to get around the type restrictions, you might as well just do new Entity({billboard: img, position: new CallbackProperty(() => ...) as any}).

Eventually, I agree with Matt from the OP, there should be a CallbackPositionProperty which just implements both interfaces.

EjiHuang commented 2 years ago

I used "@ts-ignore" to fix this. It's not a good idea.

//@ts-ignore
pickedEntity.position = new Cesium.CallbackProperty(() => cartesian, false);
mholthausen commented 2 years ago

Will there be a CallbackPositionProperty in the near future?

dukeofcool199 commented 1 year ago

what is the status of this issue? I am currently running cesium version 1.102.0 and am trying to get my entities to move using a callbackProperty. Even after forcing the type as any the behaviour I get is that cesium will silently crash. Was the accidental nature of the callback changing entity positions patched at some point?

ggetz commented 1 year ago

We don't have immediate plans to work on this item. But we'd be happy to accept a Pull Request for this feature if you have the time! Thanks for your interest!

angrycat9000 commented 2 months ago

I ran into this and have been using @ts-expect-error to work around this.

  const position = new Cartesian()
  // @ts-expect-error https://github.com/CesiumGS/cesium/issues/8944
  entity.position = position;
ggetz commented 1 week ago

This was resolved in https://github.com/CesiumGS/cesium/pull/12170. Thanks @jfayot!