ShapeModel Normal State Fix

[acpaquette]

Description

The m_normal variable in ISIS was being used to store both the "local" normal (normal obtained from more detailed shape models) and the ellipsoid surface normal. While they could remain the same it is best that the local normal be stored in one variable (m_localNormal) and the ellipsoid normal retain the current behavior and store its value in m_normal. This ensures a cleaner state within the shapemodel hierarchy but requires resetting both normals when necessary.

Related Issue

Fixes #5197

How Has This Been Validated?

Validated through existing tests

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Documentation change (update to the documentation; no code change)
• Breaking change (fix or feature that would cause existing functionality to change)
• Infrastructure change (changes to things like CI or the build system that do not impact users)

Checklist:

• I have read and agree to abide by the Code of Conduct
• I have read the CONTRIBUTING document.
• My change requires a change to the documentation and I have updated the documentation accordingly.
• I have added tests to cover my changes.
• I have added myself to the .zenodo.json document.
• I have added any user impacting changes to the CHANGELOG.md document.

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:

This work is free and unencumbered software released into the public domain. In jurisdictions that recognize copyright laws, the author or authors of this software dedicate any and all copyright interest in the software to the public domain.

• I dedicate any and all copyright interest in this software to the public domain. I make this dedication for the benefit of the public at large and to the detriment of my heirs and successors. I intend this dedication to be an overt act of relinquishment in perpetuity of all present and future rights to this software under copyright law.

[acpaquette]

@KrisBecker Some input here may be useful, I know this is not how you fixed this issue but if you can think of any pitfalls of this approach that would be ideal

[KrisBecker]

@acpaquette is it possible to get local incidence and emission angles from apps like campt and camstats? Is this the desired behavior?

Can you explain why the unit tests changed so much? Were they returning local angles before your modification?

[acpaquette]

@KrisBecker Campt and camstats have never explicitly exposed local emission and incidence angles like phocube. They were unintentionally returning local angles after #4600 was merged into ISIS due to the shape models sharing one variable for the normal. This PR separates the normal and local normal into distinct variables that need to be requested by the program/programmer, thus the change to unittests to return the localNormal vs Normal when the "local" normal is expected.

If we want local emission and local incidence exposed in camstats and campt, that is something we could do, but is not currently available. Also in #4600 the "Functional" tests truth values were changed, stating that the local emission and incidence for camstats and campt was the "truth". This PR rectifies that and the new values are extremely close to the old values. Why they aren't exact I am not sure.

Whether users want the "local" normal vs the ellipsoid normal is somewhat unclear. To some degree it's down to user expectation. If I ask for a normal with a dem or bullet shape model should I expect the "local" normal or the ellipsoid normal? Should users be able to more directly change the shapemodel of a cube without having to run spiceinit again? Some of these question may trickle into PSRMTS but may ultimately remain an issue for ISIS to tackle. Another possible avenue is that if we want all shapemodels to have access to the ellipsoid normal, it may make more sense for users to have to ask for that rather than the "local" normal.

[KrisBecker]

Unfortunately, none of the current ISIS tests for this functionality include any small, irregular bodies. In fact, there are no ISIS tests for NAIF DSK, Bullet or Embree shape models that would evaluate the impact of these changes regarding tessellated shape models. The tests used for these cases are Mars global DEM 2.5D shape models in ISIS cube projection format. The impact of modifications to ISIS shape model code for small, irregular bodies (i.e., tessellated plate shape models) lacks representation.

Triaxial ellipsoid representations generally do not adequately model small, irregular bodies. Tessellated plate models are required for more accurate cartography functionality. The implementations for ray tracing shape models we have are decidedly different from what is currently in ISIS. The emphasis is on accuracy and flexibility.

I encourage you to run more comprehensive tests that at least compare the current ISIS implementation to your changes. These tests could include campt, camstats, caminfo, footprintinit, phocube, orthorectified projections (cam2map, cam2cam), mappt, etc..., using small bodies and tessellated shape models. I would also compare results from each ray tracing system - NAIF DSK, Bullet, Embree and ellipsoids as well. Some instruments/datasets candidates would be NEAR/MSI and Eros, Hayabusa1/AMICA and Itokawa. Bennu, although small, is not that irregular. But it does have a larger variety of shape models.

I do hope we can have discussions about small, irregularly shaped body support in ISIS and the ShapeModel design specifically.

[acpaquette]

@KrisBecker I agree that those would be useful tests, I don't think this PR is a good place to add them. I will make a new issue and we can address it in future support sprints

[chkim-usgs]

Should we add a changelog entry signifying the updated ShapeModel differentiating a local normal and an ellipsoid surface normal? Looks good either way.

[acpaquette]

@chkim-usgs Yes, this should have a changelog entry. I will add that

[amystamile-usgs]

@acpaquette conflicting changelog

[Kelvinrr]

fixed the merge conflict