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

Some SceneTransforms functions docs do not correctly reflect implementation #12036

Closed
sudo-jaa opened this issue Jun 12, 2024 · 1 comment · Fixed by #12048
Closed

Some SceneTransforms functions docs do not correctly reflect implementation #12036

sudo-jaa opened this issue Jun 12, 2024 · 1 comment · Fixed by #12048

Comments

@sudo-jaa
Copy link

What happened?

The JSDoc for certain functions in the cesium\packages\engine\Source\Scene\SceneTransforms.js have JSDoc that don't completely describe the possible return values.

The affected functions are:
SceneTransforms.worldToWindowCoordinates
SceneTransforms.wgs84ToWindowCoordinates
SceneTransforms.worldToDrawingBufferCoordinates
SceneTransforms.wgs84ToDrawingBufferCoordinates

Each of these functions have JSDoc describing their return types as some variation of the following:

@returns {Cartesian2} The modified result parameter or a new Cartesian2 instance if one was not provided.  This may be <code>undefined</code> if the input position is near the center of the ellipsoid.

It is noted that each of these functions can potentially return an undefined value, but this possibility is not encoded in the type definition of the @return. Including the undefined value as a union with the successful return type is a potential resolution to this: @return {Cartesian2|undefined}.

The typescript signatures of these functions are built from the JSDoc, so when building typescript definitions from the source, the signatures of these functions end up not accurately describing the potential for failure that they have. Any consumers of cesium that are accessing these function might end up with unexpected crashes because of it.

Reproduction steps

  1. Run the script to build typescript definitions: npm run build-ts
  2. Inspect cesium\packages\engine\index.d.ts and notice that the type declarations for the above functions do not correctly reflect the implementations

Sandcastle example

No response

Environment

Browser: N/A
CesiumJS Version: cbd13cee0959d2de6e01f355dd1cd73342467102
Operating System: Windows 11 Pro

@sudo-jaa sudo-jaa changed the title Some SceneTransforms functionsdo not correctly reflect implementation Some SceneTransforms functions do not correctly reflect implementation Jun 12, 2024
@sudo-jaa sudo-jaa changed the title Some SceneTransforms functions do not correctly reflect implementation Some SceneTransforms functions docs do not correctly reflect implementation Jun 13, 2024
@ggetz
Copy link
Contributor

ggetz commented Jun 14, 2024

Thanks for pointing this out @sudo-jaa! Since some of those function were just added, this should be fixed before the next release.

jjspace added a commit that referenced this issue Jun 26, 2024
Fixes #12036 - Types for SceneTransforms functions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants