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

Deprecation Updates for SceneTransforms Methods #12156

Merged
merged 9 commits into from
Aug 30, 2024
Merged

Conversation

jvrjsanchez
Copy link
Contributor

@jvrjsanchez jvrjsanchez commented Aug 27, 2024

Description

This pull request addresses the deprecation of certain methods within the SceneTransforms module. Specifically, the methods wgs84ToDrawingBufferCoordinates and wgs84ToWindowCoordinates are being deprecated and will be removed in future versions. This update ensures that our codebase aligns with the latest changes and improves future maintainability.

  • The updated methods provide improved functionality and are part of the new design. The transition to these methods should enhance consistency and performance.
  • Ensured and tested thoroughly to confirm that the new methods behave as expected and integrated seamlessly with existing code.
  • Updated method calls in the codebase from wgs84ToDrawingBufferCoordinates to worldToDrawingBufferCoordinates.
  • Updated method calls in the codebase from wgs84ToWindowCoordinates to worldToWindowCoordinates.
PNG image Test cases passed.

Issue number and link

fixes #12059

Testing plan

  • Ran tests using the original gulpfile.js to execute them.
  • Verified that all tests for SceneTransforms pass successfully in different viewing modes (3D, ColumbusView, 2D) and frustum configurations.
  • Ensured that test output is correctly reported and errors are handled gracefully.

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @jvrjsanchez!

✅ We can confirm we have a CLA on file for you.

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks @jvrjsanchez! I have a few comments on the code that should be addressed before we can merge this.

package.json Outdated Show resolved Hide resolved
@@ -337,7 +337,7 @@ SceneTransforms.worldToDrawingBufferCoordinates = function (
* console.log(Cesium.SceneTransforms.wgs84ToWindowCoordinates(scene, position));
* }, Cesium.ScreenSpaceEventType.MOUSE_MOVE);
*/
SceneTransforms.wgs84ToDrawingBufferCoordinates = function (
SceneTransforms.worldToDrawingBufferCoordinates = function (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe SceneTransforms.worldToWindowCoordinates and SceneTransforms.worldToDrawingBufferCoordinates already have been defined. Instead, let's just remove the SceneTransforms.wgs84ToWindowCoordinates and SceneTransforms.wgs84ToDrawingBufferCoordinates.

CHANGES.md Outdated Show resolved Hide resolved
@jjspace
Copy link
Contributor

jjspace commented Aug 30, 2024

@jvrjsanchez Our monthly release happens on Tuesday and this needs to make it into that. In the interest of time because we have a long weekend I made some final touches on this to get it over the finish line. Thanks for your work on the PR.

@ggetz
Copy link
Contributor

ggetz commented Aug 30, 2024

Thanks all!

@ggetz ggetz merged commit 1669679 into CesiumGS:main Aug 30, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove SceneTransforms.wgs84To* functions in 1.121
3 participants