Skip to content
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

Add seven new files for Multi-point capabilities. GAS/3.0.4 #24

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

roseej
Copy link
Collaborator

@roseej roseej commented Jan 10, 2022

No description provided.

@roseej
Copy link
Collaborator Author

roseej commented Jan 10, 2022

remove documents per NGA guidance

// polynomial and other non-physical models. The
// imageToProximateImagingLocus method should be used instead where
// possible.
//<````````````````````````````````````````````````````````1111111111111111111111111111111111111111111111111111111111111111111111111111
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the spurious text in this comment line

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


namespace csm
{
//*****************************************************************************
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, no tab characters. Just use spaces to indent (though the other CSM code does not indent due to a namespace, so really no indent is needed here at all).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


namespace csm
{
class CSM_EXPORT_API MultiPointRasterGM : public RasterGM
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that MultiPointRasterGM should derive from RasterGM. I think it should be its own interface that models could implement by deriving from both RasterGM and MultiPointRasterGM. That way it is optional, and clients can determine if it has this interface by doing something like MultiPointRasterGM* mpModel = dynamic_cast<MultiPointRasterGM*>(someRasterModel); if (mpModel) { ... }

Doing this allows for other classes like MPRGWrapper to only need to implement MultiPointRasterGM methods and not pass-throughs for all of the RasterGM methods.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiple designs were considered including this one.

//<

virtual MultiEcefVector multiGetIlluminationDirection(const MultiEcefCoord& groundPts) const = 0;
virtual MultiDbl multiGetImageTime(const MultiImageCoord& imagePts) const = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are all these multi-point accessors really that useful? I suppose they probably aren't that hard to implement for plugins, but is there really a need to get the image times or illumination directions of lots of points in a single call? And will that really be that much faster than doing lots of calls? I'm worried about this interface becoming too complicated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these in particular were requested by JKarp.

// Additions were made to the CSM API in CSM 3.0.2 to handle point clouds.
// The previous API did not change. Previously this code was in a separate
// library, but has now been migrated into a single library as of
// CSM3.0.3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably need a better description here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

// ----------- ------ -------
//
// 22FEB2018 JPK Modified to use existing csm macros (since
// point cloud is no longer in a separate library).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely incorrect SW history

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@sminster sminster changed the base branch from MultiPoint to master January 10, 2022 18:58
@sminster
Copy link
Collaborator

I don't think it's a good idea to add PDF files to this repository like that. Adding binary files just bloats the repository, making it slower to clone. Please remove them from this PR and do not do that in the future.

@roseej roseej self-assigned this Mar 4, 2022
@roseej roseej requested a review from sminster March 4, 2022 14:26
@sminster
Copy link
Collaborator

I tried compiling these changes and got compile errors. Of course, that was after I updated the Makefile to include the new files. So first, you need to modify the Makefile to include the files. Then you need to fix the compile errors, mostly in csmMultiPoint.h. I think they are a result of using C++11 features. We could make that a required standard, but that would mean adding something like -std=c++11 to the COPTS in the various Linux makefiles at least.

Also, I think you should do a git rm on the two PDFs. That won't really remove them from the Git history, but I don't think we want those two in what people see all the time.

@sminster sminster added the csm4 Targetted for CSM4.0 label Sep 15, 2022
@sminster
Copy link
Collaborator

sminster commented Aug 7, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
csm4 Targetted for CSM4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants