Cesium: Sandcastle examples have deprecation warnings

[mramato]

After #5848 was merged, there are now a bunch of deprecation warnings in Sandcastle examples. We should probably fix them before today's release. (It's probably easiest to go through each example and fix them as you go.

CC @ggetz

[bagnell]

It looks like #5848 was a breaking change, not a deprecation. Should we remove the deprecationWarnings or just close this?

[hpinkos]

A deprecationWarning doesn't seem appropriate. Maybe we should change this to a oneTimeWarning with a clear description of how to fix the app.

[ggetz]

The warning's already include the instructions, I can just swap the deprecationWarnings out for oneTimeWarings

[hpinkos]

Sounds good to me @ggetz !

[mramato]

I'm not sure any warning in the code makes sense because the warning will happen even if the code is correct. I would rather not have anything at all, we don't normally have console warnings for breaking changes.

[bagnell]

The deprecation warnings are also printed multiple times because they all use the same identifier. You should also switch them to have the same identifier.

[ggetz]

But this has changed the behavior pretty commonly used functions, I think some kind of warning is necessary. I'll change them to to use all the same identifier though, so we should only get that one warning.

[mramato]

But this has changed the behavior pretty commonly used functions, I think some kind of warning is necessary. I'll change them to to use all the same identifier though, so we should only get that one warning.

The problem with this approach is that it causes Sandcastle to show the warning and switch the tab for a ton of examples (even ones that don't use HPR directly). And as I stated, the fact that the warning will show up even in new code using the correct functionality will lead to user confusion.

The real answer is that this changed should have never been merged in this form, we rarely break something in a way where there is no temporary backwards compatibility, but when we do we do not spit out a message in the console. That's what CHANGES is for.

[denverpierce]

This change kinda caught me by surprise, I didn't see the notice in the issue. I'd prefer _some_ kind of notice that the functionality has changed rather than nothing, so that when I upgrade I'll be more likely to realize there are issues on a cursory check.

[ggetz]

We went forward with a breaking change instead of deprecating old functions and introducing new ones because the old function names were the one we wanted to keep. See https://github.com/AnalyticalGraphicsInc/cesium/pull/5809 for more discussion.

[hpinkos]

The real answer is that this changed should have never been merged in this form, we rarely break something in a way where there is no temporary backwards compatibility

I'm honestly surprised this got merged in as well. This is such a huge breaking change, it's going to effect literally everyone using our library. I don't remember a ton of people complaining that clockwise was incorrect on the forum. We should have reached out to the community before going ahead with this. I anticipate a ton of people asking why their data no longer works after they try to upgrade to 1.38

[ggetz]

So would we prefer to remove those changes or include a warning one time?

[ggetz]

I opened https://github.com/AnalyticalGraphicsInc/cesium/pull/5870 if it's the latter.

[hpinkos]

I personally wouldn't want to include this change in the release. Even though CCW would have probably been the better choice when we added HPR, I don't think it makes sense to change it now without warning the community first. It just doesn't seem like there's adequate justification for a breaking change of this magnitude.

@mramato @bagnell @lilleyse @pjcozzi what are your thoughts?

[denverpierce]

The decision is essentially an arbitrary one. Even in the aviation industry many of the conventions aren't consistent. As an example, airframe design is often done in one reference, while aircraft plumbing systems are designed with a positive Y-axis from tail to nose (I believe).

Cesium has used a a consistent convention for some time, and it should be left as-is to preserve continuity in the API.

[mramato]

I'm not against the changes, just the handling of a breaking change with no deprecation timeframe and a message that pops up even when the user isn't explicitly using the functionality. (especially since it makes Sandcastle really annoying to use.

[mramato]

@pjcozzi since you were the one that ultimately merged this change, can you please chime in here?

[bagnell]

If it really is arbitrary, I'd like to remove this from the release.

@denverpierce I made the heading counter-clockwise in the initial PR because according to the FAA Pilot's Handbook:

The true heading (TH) is the direction in which the nose of
the aircraft points during a flight when measured in degrees
clockwise from TN.

The only difference is the angle is measured in radians to be consistent with the rest of the API.

Are there different conventions for different countries as well?

[denverpierce]

It just depends on what reference you want. As far as aircraft headings go, I believe the FAA guidelines are consistent.

According to wikipedia, magnetic compass point points are quite varied, but Warsaw Pact countries commonly used one system that was CCW from magnetic north.

I'm not familiar with any other systems for aircraft heading.

[bagnell]

I'm going to pull the changes so we can start the release. I'll leave them in a branch where we can continue the discussion.

[hpinkos]

Thanks for everyone's input! We can continue discussing how we may want to approach this in #5666