Initial steps to modularize UsgsAstroLsSensorModel::losToEcf #169
Conversation
…-by-reference parameters to be consistent with rest of code
|
RE moving to a utility file. @acpaquette has already started doing that with distortion. I think we can just do all of that after this gets merged. |
There was a problem hiding this comment.
Everything looks good to me. We can either iterate on this PR, or merge this and then add more PRs to iterate after @acpaquette gets his in.
| const double& lineUSGS, | ||
| const double& sampleUSGS, | ||
| double &distortedLine, | ||
| double& distortedSample) const; |
There was a problem hiding this comment.
We should get rid of all the USGS/isis variable names. I'd much rather see lineCCD, sampleCCD than lineUSGS, sampleUSGS.
| const double& isisNatFocalPlaneX, | ||
| const double& isisNatFocalPlaneY, | ||
| double& isisFocalPlaneX, | ||
| double& isisFocalPlaneY) const; |
There was a problem hiding this comment.
Same here, better variable names
|
|
||
|
|
||
| // Given a time and a flag to indicate whether the a->b or b->a rotation should be calculated | ||
| // uses the quaternions in the m_quaternions member to calclate a rotation matrix. |
There was a problem hiding this comment.
I would much rather see this just compute the rotation matrix and then the user can transpose it for the inverse rotation if they want to. Should we consider adding a linear algebra package to make this way better?
There was a problem hiding this comment.
At some point I think we were trying to (or required to?) avoid adding dependencies? If we can, adding a linear algebra package would be great.
There was a problem hiding this comment.
I'm curious how the team feels about adding a linear algebra package like armadillo @Kelvinrr @acpaquette @jlaura @thareUSGS
There was a problem hiding this comment.
I didn't respond earlier to your other point: I did change this to just calculate the rotation matrix, and updated the caller that used invert=true to apply the transpose.
src/UsgsAstroLsSensorModel.cpp
Outdated
| attCorr[5] = -sin_a * cos_c + cos_a * sin_b * sin_c; | ||
| attCorr[6] = -sin_b; | ||
| attCorr[7] = sin_a * cos_b; | ||
| attCorr[8] = cos_a * cos_b; |
There was a problem hiding this comment.
This is a rotation matrix calculated from euler angles. It would be nice to have a function similar to the one for quaternions that computes a rotation matrix from euler angles.
There was a problem hiding this comment.
Sure, this sounds like a good idea, and will do.
|
|
||
| void calculateRotationMatrixFromQuaternions( | ||
| const double& time, | ||
| const bool& invert, | ||
| double cameraToBody[9]) const; | ||
|
|
||
| void calculateRotationMatrixFromEuler( | ||
| double euler[], | ||
| double rotationMatrix[]) const; |
There was a problem hiding this comment.
This needs to be a reference to return properly?
There was a problem hiding this comment.
This is how UsgsAstroLsSensorModel::determineSensorCovarianceInImageSpace made use of it's array (sensor_cov) and successfully uses it in the same way (updating it within the method call).
There was a problem hiding this comment.
@jessemapel Doesn't this implicitly pass a pointer to the first element of the array? It's not exactly the same thing as pass by reference, but doesn't it function similarly in practice?
There was a problem hiding this comment.
@kberryUSGS That's right, I forgot about how arrays are passed around.
…eter. Now, will always return the matrix that rotates the in the same direction as the quaternions.
... along with some of
computeViewingPixelas a bonus. This PR does not add tests for the newly added functions. Looking for feedback about the way this is modularized first.Also, notably this doesn't (yet) pull these functions out into a separate file or make any attempt for portability / usability between the linescanner and framer sensor models, though this would be good to do in the future. This should fix #166 and includes steps toward #165.
When this does get merged, please squash and merge. The commit history includes converting back-and-forth between unix-style and dos-style carriage returns and this really doesn't need to be recorded.