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

Quaternion confusion #1342

Merged
merged 6 commits into from
Dec 18, 2013
Merged

Quaternion confusion #1342

merged 6 commits into from
Dec 18, 2013

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Dec 16, 2013

So this has been a problem ever since 3004c02. The easiest way to see the issue is to load LotsOfSensors and then scrub all the way to the beginning of the timeline. This causes the orientation to get messed up. There are multiple issues here.

First, there is an extra call to Quaternion.conjugate at the end of Quaternion.unpackInterpolationResult. Therefore, whenever you sampled a quaternion you got a different answer than if you landed on a sample directly (as you do at the very beginning of the timeline in the Lots of Sensors example). However, you would expect the behavior to be the opposite (only correct when we are directly on a sample time and wrong when interpolating). This indicates that there was another bad conjugation somewhere else that was causing two wrongs to make a right.

It turns out that 3004c02 causes an implicit conjugation of the Quaternion in Matrix3.fromQuaternion. @kring wasn't sure if this was correct or not (though @bagnell says it is definitely intentional).

Long story short (too late) this pull fixes the current issue and a future PR can change the behavior of Matrix3.fromQuaternion if desired.

CC @fstoner @pjcozzi

… exposes an implict conjugation in Matrix3.fromQuaternion which needs to be dealt with in the visualizers.
@mramato
Copy link
Contributor Author

mramato commented Dec 16, 2013

@kring and @fstoner just finished a discussion in which it was decided that Matrix3.fromQuaternion is correct for the graphics world and that we should change CZML to provide the conjugate of what it is providing now (and remove the extra conjugate I've added to the visualizers. I'll make the changes and let everyone know when this is ready to be merged.

@mramato
Copy link
Contributor Author

mramato commented Dec 16, 2013

This is ready. NOTE: All existing CZML files that use sensors or ellipsoids will have to be updated. I'll open a PR on the web-site to do this for anything we have there.

@emackey
Copy link
Contributor

emackey commented Dec 17, 2013

I know that both CZML and Cesium itself are "in beta," but if we're going to break backwards compatibility, I say we should fully break it: Make it so the old quaternions can't possibly load, rather than load and display inaccurate data. That's AGI's way, we'll crash before we dare show you an incorrect result. A certain project I worked on has a stash of old CZML files saved on the corporate network, for use as bug report attachments and potentially for regression testing. It's not enough just to post a public message asking everyone to regenerate every saved CZML file that ever used orientation. The message must pop up in the user's face at the moment the user attempts to load an obsolete file.

Beta or not, CZML is already deployed to multiple customers' production environments. It needs to be able to evolve, and it can have breaking changes, but it needs to fully break when that happens. Maybe that means attaching a version number, rename "orientation" to "orientation2" or "attitude" or something. But we can't claim that the "beta" classification allows us to turn old results into analytical errors.

@mramato
Copy link
Contributor Author

mramato commented Dec 17, 2013

Customers choose when to upgrade to a new Cesium release, it's not forced on them. Unfortunately, Cesium is in a state where we break backwards compatibility more often than I would like; we've even done it before with CZML (and I can gaurentee we will end up breaking it again). There will be a time when we start to version things and promise that the format is stable, but we're not there yet.

@emackey
Copy link
Contributor

emackey commented Dec 17, 2013

Great, so let's really break it. Make it so old files with orientation won't load, or won't use the orientation block.

When a paying customer sends us a CZML file to debug 6 months from now, we shouldn't have to ask what version produced it, or whether the quaternions were conjugated. It should be apparent from looking at the file. Otherwise, all non-streamed CZML will be forever ambiguous. Paying customers use file-based CZML, particularly for bug reports.

We can't just ship incorrect analysis and hide behind the Beta label. People need to be able to trust our numbers, that's why they use us.

@mramato
Copy link
Contributor Author

mramato commented Dec 17, 2013

We have no paying customers and this is visualization, not analysis. This is not the first time we've broken backwards compatibility with CZML, so I'm not sure why you are making a stink over it. The only real way to prevent the file from loading would be to not load ANY CZML files created prior to b24, which doesn't make sense since barely any files are affected by this. This change affects a very small percentage of CZML users and if anyone ever contacts the mailing list with a problem, it will be easily apparent if this is the cause of it. We can't commit to backwards compatibility now because that means that every time we need to change something we have to worry about it, that's something I'm not prepared to commit to yet, especially since there are other changes in the pipeline. Anyone writing CZML now is doing it with custom code (this change has zero-effect on czml-writer), it's perfectly acceptable to ask them to regenerate there data. This is an easy change to make and not really different than any other Cesium breaking change. At some point, I am all for version information and BC, but not yet.

And if you insist on wearing your AGI hat for this conversation, I know for a fact it will have no real impact on any of the Cesium projects you are working on once the changeover is made.

@mramato
Copy link
Contributor Author

mramato commented Dec 17, 2013

After a bunch of offline debate, we decided to basically punt on any CZML changes for now. We're going to fix this bug and leave the CZML quaternion representation as is (by conjugating during CZML load time). In the near future, we hope to revisit CZML in general and make any breaking changes in one big swoop.

@mramato
Copy link
Contributor Author

mramato commented Dec 17, 2013

@emackey This should be good to go.

emackey added a commit that referenced this pull request Dec 18, 2013
@emackey emackey merged commit 5e69628 into master Dec 18, 2013
@emackey emackey deleted the quaternion-confusion branch December 18, 2013 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants