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

Change Transforms.headingPitchRollToFixedFrame to use new HeadingPitchRoll #4468

Closed
pjcozzi opened this issue Oct 20, 2016 · 5 comments
Closed
Labels
breaking change cleanup good first issue An opportunity for first time contributors

Comments

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 20, 2016

https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/Transforms.js#L475-L481

Deprecation policy is here: https://github.com/AnalyticalGraphicsInc/cesium/tree/master/Documentation/Contributors/CodingGuide#deprecation-and-breaking-changes

@twpayne low-hanging fruit for the bug bash if you are interested.

CC #4047

@pjcozzi pjcozzi added cleanup good first issue An opportunity for first time contributors breaking change bug bash labels Oct 20, 2016
@twpayne
Copy link
Contributor

twpayne commented Oct 21, 2016

I'm looking at this now.

@twpayne
Copy link
Contributor

twpayne commented Oct 21, 2016

AFAICT we already have aircraftHeadingPitchRollToFixedFrame which implements headingPitchRollToFixedFrame but taking a HeadingPitchRoll object instead of separate heading, pitch, and roll values.

Which of the following options would you prefer?

  • Mark headingPitchRollToFixedFrame as @deprecated and replace all calls to it in the Cesium codebase with calls to aircraftHeadingPitchRollToFixedFrame as per the deprecation policy.
  • Modify headingPitchRollToFixedFrame to take either a HeadingPitchRoll object or separate values (will require some arguments munging). This maintains a cleaner API but there will be a performance hit.
  • Remove the original headingPitchRollToFixedFrame, rename aircraftHeadingPitchRollToFixedFrame to headingPitchRollToFixedFrame, and use HeadingPitchRoll objects throughout the codebae. This is arguably the cleanest but violates the deprecation policy.
  • Something else?

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Oct 21, 2016

Mark headingPitchRollToFixedFrame as @deprecated and replace all calls to it in the Cesium codebase with calls to aircraftHeadingPitchRollToFixedFrame as per the deprecation policy.

The semantics of "aircraft" are a bit awkward to use internally; I would rather deprecate both of these (if the aircraft version has already shipped) and replace with a new more generically named function.

@twpayne
Copy link
Contributor

twpayne commented Oct 21, 2016

As naming things is one of the two hard things in computer science, any suggestions for the new more generically named function?

  • headingPitchRollToFixedFrame (the existing function name) seems to me to be the most apt, but this will require the arguments munging above to maintain backward compatibility.
  • hprToFixedFrame includes a rarely-used abbreviation and so violates the naming guidelines in the coding guide.
  • 🤔 ?

Note that the "aircraft" functions were made public in 4aa7eee, which was only merged into master yesterday (#4047), and so they have not yet shipped in a release.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Oct 21, 2016

headingPitchRollToFixedFrame (the existing function name) seems to me to be the most apt, but this will require the arguments munging above to maintain backward compatibility

We haven't done this before, but perhaps we could just "deprecate the arguments" instead of the function. Since this is not a widely used function, I think we should give this a shot and then deprecate over 3 releases since this approach is new to us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change cleanup good first issue An opportunity for first time contributors
Projects
None yet
Development

No branches or pull requests

2 participants