Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[request] generic Property type #8930

Open
rot1024 opened this issue Jun 8, 2020 · 7 comments
Open

[request] generic Property type #8930

rot1024 opened this issue Jun 8, 2020 · 7 comments

Comments

@rot1024
Copy link

rot1024 commented Jun 8, 2020

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:

  • I know type definitions are generated from JSDocs, but I don't know how to represent generic type in JSDoc...
  • Setting the default type parameter to any is good idea because it will prevent to compromise compatibility.
@rot1024 rot1024 changed the title [request] Entity [request] generic Entity type Jun 8, 2020
@rot1024 rot1024 changed the title [request] generic Entity type [request] generic Property type Jun 8, 2020
@mramato
Copy link
Contributor

mramato commented Jun 8, 2020

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
Copy link
Contributor

thw0rted commented Jun 9, 2020

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
Copy link
Contributor

thw0rted commented Jun 9, 2020

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
Copy link

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
Copy link
Contributor

thw0rted commented Sep 1, 2020

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
Copy link

ThisNameNotUsed commented Sep 2, 2020

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
Copy link
Contributor

thw0rted commented Sep 2, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants