csm icon indicating copy to clipboard operation
csm copied to clipboard

Reference frame enhancements non ecef models

Open roseej opened this issue 3 years ago • 2 comments

roseej avatar Feb 02 '22 21:02 roseej

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 ObjectSpaceRasterGM is just a more generic RasterGM in that it can work with the 3 dimensional coordinates that are in a coordinate system other than ECEF. So is the idea that ObjectSpaceRasterGM should eventually replace RasterGM?

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 dynamic_cast to models that support this class, and then query the information. This effectively lets both models and SETs easily opt into getting this information without having to do major re-writes.

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.

sminster avatar Feb 22 '22 18:02 sminster

Before this PR can be merged, the new files must be added to the Makefile.

sminster avatar Aug 07 '23 16:08 sminster