DM-10764: Rename Transform::of and Mapping::of to ::then #240
Conversation
Change `Transform::of(first)` to `Transform::then(next)` to improve readability.
| * this Transform to the result. Its inverse shall first apply the | ||
| * inverse of this Transform, then the inverse of `first`. | ||
| * @tparam NextToEndpoint the "to" Endpoint of `next` | ||
| * @tparam FromEndpoint the |
There was a problem hiding this comment.
FromEndpoint is not a template parameter of then. I'm surprised Doxygen didn't complain.
| * have the same number of axes. | ||
| * @exceptsafe Provides basic exception safety. | ||
| * | ||
| * More than two Transforms can be combined in series. For example: | ||
| * | ||
| * auto skyFromPixels = skyFromPupil.of(pupilFromFp) | ||
| * .of(fpFromPixels); | ||
| * auto pixelsToSky = pixelsToFP.then(fpToPupil.then(pupilToSky)); |
There was a problem hiding this comment.
Is this really more natural than pixelsToFP.then(fpToPupil).then(pupilToSky)?
There was a problem hiding this comment.
Agreed. I'll fix it on a new ticket.
| declareMethodTemplates<GenericEndpoint, FromEndpoint, ToEndpoint>(cls); | ||
| declareMethodTemplates<Point2Endpoint, FromEndpoint, ToEndpoint>(cls); | ||
| declareMethodTemplates<SpherePointEndpoint, FromEndpoint, ToEndpoint>(cls); | ||
| declareMethodTemplates<FromEndpoint, ToEndpoint, GenericEndpoint>(cls); |
There was a problem hiding this comment.
I find this form slightly less readable because it puts the most important information at the end, though I can see why you changed it.
There was a problem hiding this comment.
Yes, I thought about that. I'm still happier with it because I can instantly see that it matches the header file and other code.
| @@ -535,7 +535,8 @@ int warpImage(DestImageT &destImage, SrcImageT const &srcImage, | |||
| std::vector<double> const srcParentToLocalVec = {static_cast<double>(-srcImage.getX0()), | |||
| static_cast<double>(-srcImage.getY0())}; | |||
| auto const srcParentToLocalMap = ast::ShiftMap(srcParentToLocalVec); | |||
| auto const localDestToSrcMap = srcParentToLocalMap.of(destToSrc.getFrameSet()->of(destLocalToParentMap)); | |||
| auto const localDestToSrcMap = | |||
| destLocalToParentMap.then(*(destToSrc.getFrameSet())).then(srcParentToLocalMap); | |||
There was a problem hiding this comment.
Oops -- should Transform's constructors be explicit?
For that matter, why not just use destToSrc?
There was a problem hiding this comment.
Transform's constructors are explicit because I usually make all constructors explicit under the argument that it's harmless and can avoid trouble (e.g. if a 2 argument ctor is changed to be a 1 argument ctor). But why do you ask?
As to why not just use destToSrc: it maps parent coords to parent coords, but C++ indexing of images is done using local coords. The above is needed for the code to work when xy0 is non-zero.
There was a problem hiding this comment.
If you mean "why not use destToSrc directly instead of getting the FrameSet from it?"....that's because everything else is a Mapping and I don't want to mix Mapping and Transform in my concatenation. And I don't want to use Transforms when Mappings will do just fine. I suppose I could change that.
There was a problem hiding this comment.
Aha, I missed the part where destLocalToParentMap was not a Transform. It all makes sense now. 🙂
There was a problem hiding this comment.
That's why I ended the name with "Map". A bit of Hungarian notation that was clearly not as helpful as I'd hoped.
Change
Transform::of(first)toTransform::then(next)to improve readability.