Reference frame enhancements non ecef models #27
Conversation
Address Scott Minster comments
fix previous update which inserted header file code into .cpp file. This addresses Scott's comments to the .cpp file.
Address Scott's comments.
8 files for non-ECEF sensor model enhancement.
|
This biggest issue that I have with this change is that it is a lot of code, but the interfaces are almost exactly the same as what we currently have, just with different names. Are plugin writers going to want to duplicate their implementations but replace "Ecef" with "ObjectSpace"? Are SET writers going to want to do the same? It almost feels like If that is the case, I think we need to go all out and change the "object space" definition to be more flexible. Right now, it is essentially an enumeration. Maybe it should be instead something more like a Well Known Text (WKT) definition of the 3d space? A more minimalist change would be to not have "object space" copies of all the ECEF structures but to just reuse those structures in the current classes, but interpret them differently. I realize that this can create a little cognitive dissonance (getting an ECI point in a structure called EcefCoord), but it would reduce a lot of duplication of classes. So instead, I would suggest another class that the plugnis could implement that could get/set information on 3d space that the RasterGM returns. The SET could do a So overall, I'm not too happy with this proposed change because I think we could accomplish this better, though I don't think it will really hurt anything that currently exists. |
| { | ||
| enum class CSM_EXPORT_API ObjectSpaceType : unsigned char | ||
| { | ||
| LSR, |
There was a problem hiding this comment.
If the ObjectSpaceType is LSR, what is the ground reference point (that corresponds to (0,0,0) in the LSR space)?
There was a problem hiding this comment.
The interfaces are not the same. They have different arguments specific to non-ECEF object spaces.
No, these changes do not impact existing sensor models.
The idea is not that ObjectSpaceRasterGM should replace RasterGM.
The idea of the object space coordinate etc. structures being eventually generalized to include ECEF, so that one day all models would just be object space models, has been discussed and it was decided that this is too large of a change for a minor release. If these changes are going to be part of a major release (taking more time) then that could be done. The definition of "minor" versus "major" release is dependent on maintaining backward compatibility. CSMWG needs to weigh in.
No, we are not going to reuse ECEF structures and interpret them differently.
There are no "major rewrites" of existing models induced by these changes. As for SETs, we are assuming that new SET functionality will be needed for these new non-ECEF models. If someone is writing a new SET to exploit close-range or NEI imagery, or adding this functionality to a new SET, new code will have to be written regardless of the model interfaces.
The new models do have get methods that describe the object space type, which is an enum.
There was a problem hiding this comment.
I do like in general how this is pushing CSM in a more general direction. I just don't know if the duplication is the right way to do it. I think if we do go with a duplicative way, it should be in such a way as "this is the future, you really should be converting to this interface". That would be a point release, and a major release would deprecate/remove the existing RasterGM interface.
So in some ways, I don't think this is generic enough. The enum in particular is limiting, because you can only use those values. Would it be possible to describe the object spaces sufficiently with something like WKT (well known text)? That would allow for many types of coordinate systems to be described. Would it be possible to have a "set" method for the object space coordinate system, at least as a suggestion to the model that could return true/false if the requested CS is available? For example, what if you wanted an LSR centered about a different location?
btw, I also see now that the replacement of the double height in the 2d to 3d conversion with a ProjectionParameters is also a crucial difference between the existing RasterGM and the new ObjectSpaceRasterGM. I'll have to think about this more...
| // get the geometry type (enum) | ||
| //< | ||
|
|
||
| const double &getRange() const; |
There was a problem hiding this comment.
It's a little inconsistent that the "get" method for the range returns a const reference but the "set" method does not take one. In this case, since the type is a POD type (double), I don't think there is much advantage to doing the const reference. So either way is fine, but we should be consistent.
A style note: I personally prefer the & (or * for pointers) to be on the data type, not the name (parameter name or function name). I realize that it is more /technically/ correct to put them on the name, but it makes it easier for me to think of them modifying the type. And it only really matters if you are declaring multiple variables in a single statement like int* a, b where in this case a would be a pointer and b would not. Since that doesn't apply to function names or parameters, you should put the modifier closer to the type name.
| // Coordinate values are in units of meters. | ||
| //< | ||
| //*** | ||
| struct CSM_EXPORT_API ObjectSpaceCoordinate |
There was a problem hiding this comment.
I do like sminster's suggestion to simply
typedef EcefCoord ObjectSpaceCoordinate;
etc when possible. That would give SETs the option to interact with these new classes with existing code and/or same between ecef and close range vs writing separate code for interacting with close range vs ecf.
|
Before this PR can be merged, the new files must be added to the |
No description provided.