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.91k stars 3.48k forks source link

[request] generic Property type #8930

Open rot1024 opened 4 years ago

rot1024 commented 4 years ago

I suggest that adding a type parameter to Property class type in Cesium.d.ts:

export class Property<T = any> {
    constructor();
    readonly isConstant: boolean;
    readonly definitionChanged: Event;
    getValue(time: JulianDate, result?: T): T;
    equals(other?: Property): boolean;
}

Why:

Currently Property's value is any, so it is loose. We can use the types to more tightly constrain the content of a Property.

Notes:

mramato commented 4 years ago

Thanks @rot1024 I've been trying to carve out the time to write up this issue myself. We definitely want Property to be a generic and to be able to express this through the documentation. We started experimenting with generics for Event in #8903 (tsd-jsdoc can express them with @template) so we are hopeful that we'll be able to do the same thing for Property sooner rather than later.

thw0rted commented 4 years ago

Matt, if I made Property generic with a default the same way I did with Event, would there be any reason to hold back on it? Should it be a separate PR or would it make sense to just pile it in to #8903 ?

thw0rted commented 4 years ago

I put together this branch to just kind of see what it would look like. I stumbled in a couple of places that I did not expect:

TimeIntervalCollection is tied to a lot of the Property implementations, so that's going to have to be generic as well eventually. I left "TODO" comments in relevant places.

Packable didn't really work as intended in TypeScript. There's a lot of history around this (like this and this) but the short version is that there is no syntax for constraining the generic type parameter to statically implement a method. If you look at the typings generated today, Packable winds up as an empty interface and a namespace with some static methods. It's certainly possible that somebody more TypeScript-smart than I am could crack this.

The docs don't currently do anything with the @template tag, so you're left with placeholder type names that don't link to anything. This happened with Event in my other branch as well. Maybe the description should explicitly say something like "The interface for all properties, which represent a value of type WrappedType, that can optionally vary over time" or similar.

ThisNameNotUsed commented 4 years ago

I think this comment goes here in this thread?

I am leading a large project to move an old C++ 3D app to a Cesium based one. We are trying use/learn typescript but we want it to be as close to vanilla JS as possible so we can switch when we want for quick prototyping purposes.

Just a few days ago I got TS working but there was an uneasy amount of redness that filled my VSCode editor concerning Cesium. This is of particular concern:

image

What is the proper way to address this and how far does the method to get rid of these red squiggles diverge from plain JS?

thw0rted commented 4 years ago

I believe you can wrap the call to HeadingPitchRoll.fromDegrees or the Cartesian3 constructor, inside a new ConstantProperty(...). Basically, Property is a pretty lightweight wrapper around values that allows them to vary depending on simulation time. If your value doesn't vary over time, you can pass the value directly to whatever expects a property, and generally the method or constructor will wrap the value in a ConstantProperty for you before using it.

The workaround for now is to wrap it yourself before passing it, which will make TypeScript happy (only a Property is allowed) and it's totally vanilla JS. In the long run, I'd like to see the type data for Entity#ConstructorOptions accept orientation: Property | HeadingPitchRoll, as well as viewFrom: Property | Cartesian3. (I'm not positive why it doesn't do that now?)

ThisNameNotUsed commented 4 years ago

Thank you very much. That is what I ended up doing. The value does change over time, though, so I am looking for how to approach that elegantly.

I too am confused about the requirement of 'Property' everywhere and pretty much nothing else. Please read on.

Before I wrapped all of these fields of polyline...

image

... they all threw the same generic type error asking for an implementation of Property ... image

...which seems like it could be way more specific.

Just these 4 fields of Entity.polyline took me almost an hour of frustration stemming from trying to find the right child class of Property. I had to go to the documentation and click through each child of Property and all their children to find something that sounds right. I don't see why these fields can't be more specific about the implementation of Property that they require. Is there some Cesium documentation somewhere I am missing that would explain the reasoning for all of this or could we just make the required class more specific or flexible (as you stated before)?

thw0rted commented 4 years ago

Thomas, I don't want to be the Github Police but this is getting into help/support rather than discussion about the feature request. You should stop by the community site when you have more questions.

If you stick with Typescript, you'll get used to the "is not assignable to" errors. It's worded that way because TS uses duck typing, so it can't just say "expected Property, got Boolean", it's all based on whether the r-value meets the constraints of the l-value.

As for types of Property, there are a few domain-specific implementations (like MaterialProperty for textures or PositionProperty for colors), but generally if it says it wants a property, you just have to know the underlying (wrapped) type -- though I agree with you that the documentation often does not make this clear, and it would help us end-users to have the wrapped type spelled out as a generic parameter. Then, you can add a time dimension using any Property implementation or even roll your own.

When I was getting started, I found the Sandcastle demo collection to be of great value -- that's where I discovered e.g. SampledPositionProperty. It can also help you find out what wrapped-type a property or method is expecting when it says it wants a "Property".