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

Type error in ClippingPolygon.equals #12389

Open
rruhdorfer opened this issue Dec 23, 2024 · 1 comment
Open

Type error in ClippingPolygon.equals #12389

rruhdorfer opened this issue Dec 23, 2024 · 1 comment

Comments

@rruhdorfer
Copy link

What happened?

The static ClippingPolygon.equals() method is typed wrong.
The parameters left and right should be typed as ClippingPolygon not Plane.

Reproduction steps

  1. Create two ClippingPolygon
  2. Try comparing them using ClippingPolygon.equals(left, right)
  3. TypeScript will throw an error:

Argument of type 'ClippingPolygon' is not assignable to parameter of type 'Plane'.
Type 'ClippingPolygon' is missing the following properties from type 'Plane': normal, distance

Sandcastle example

No response

Environment

Browser: -
CesiumJS Version: 1.124.0
Operating System: -

@javagl
Copy link
Contributor

javagl commented Dec 25, 2024

The typing error looks uncontroversial for me, and I opened a PR at #12394 to fix this.

Broader questions:

  1. Usually, classes that have a ClassName.equals function also have a ClassName.prototype.equals function that delegates to the first one. Should this be added here as well? (I'd be in favor of "yes")
  2. Usually, for these ClassName.equals functions, both parameters are optional. There are very few exceptions to that, and Plane (which this equals function was copied-and-pasted from) is one of these few. For some others (like Spherical or OrientedBoundingBox), the parameters are optional, but not declared to be optional via the [brackets]. This should be made more consistent...
  3. What is "equality"? 🙂

The last one refers to the fact that one could very reasonably expect the following to print true:

const polygonA = new Cesium.ClippingPolygon({
  positions: [
    new Cesium.Cartesian3(0, 1, 2),
    new Cesium.Cartesian3(2, 3, 4),
    new Cesium.Cartesian3(4, 5, 6),
  ]
});
const polygonB = new Cesium.ClippingPolygon({
  positions: [
    new Cesium.Cartesian3(0, 1, 2),
    new Cesium.Cartesian3(2, 3, 4),
    new Cesium.Cartesian3(4, 5, 6),
  ]
});
console.log(Cesium.ClippingPolygon.equals(polygonA, polygonB));

But it prints false, because it is comparing arrays via ===. I'd consider to 1. fix this, and 2. add an equalsEpsilon function, like it is done in similar classes...

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

2 participants