-
Notifications
You must be signed in to change notification settings - Fork 168
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
CSM Naif Radii #5414
base: dev
Are you sure you want to change the base?
CSM Naif Radii #5414
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still needs a changelog entry.
if (target->radii().size() == 0) { | ||
// get radii from CSM | ||
csm::Ellipsoid targetEllipsoid = csm::SettableEllipsoid::getEllipsoid(m_model); | ||
std::vector<Distance> radii = {Distance(targetEllipsoid.getSemiMajorRadius(), Distance::Meters), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the same as: if no camera model, default to ellipsoid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat, the difference here is in the Target changes where we try to read the radii from the naifwords first. If we can't, we never set the radii so we should pull the radii off the CSM model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how often are those different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The isds generated by ALE produce the same semi-major and semi-minor radii but don't capture a third radii if present. For our more common targets (Moon and Mars), the radii are often just a single radii, or a bi-axial ellipsoid. For less common targets, specifically small bodies, we often use a tri-axial radii and that third radii would not be captured by the CSM model
From the issue, the problem is that CSMCamera doesn't use a system shapemdoel (DEM), I dont see where in this code it addresses that? Probably missing something though. |
@Kelvinrr I related it to that issues but it doesn't fix it. This is a related but separate issue as CSM only stores a bi-axial ellipsoid while ISIS has the potential to use a tri-axial ellipsoid. For the Moon and Mars this isn't an issue but it may be an issue when interacting with small bodies that often define an A, B, and C radii |
tests look good, add a changelog entry and I'll approve this. |
Unable to run tests until merge conflicts have been handled. |
510cf52
to
9b09676
Compare
The following tests FAILED:4966 | 1350 - CSMCameraFixture.SetImage (Failed) |
Description
Allows CSMCamera to pickup naif target radii if they exist on the cube
Related Issue
#5385
How Has This Been Validated?
Validated locally
Types of changes
Checklist:
Licensing
This project is mostly composed of free and unencumbered software released into the public domain, and we are unlikely to accept contributions that are not also released into the public domain. Somewhere near the top of each file should have these words: