ENH: Update SpatialObjects for MetaIO v1

[aylward]

When reading/writing spatial objects via Meta, provide the option of using MetaIO version 1, which fixes bugs in the original MetaIO and SpatialObject implementations.

Specifically,

• ElementOrigin, ElementDirection, and ElementSpacing are used for the image index to physical space transform specification
• TransformMatrix, Position, and CenterOfRotation are used for the object-to-parent transform in the spatialobject scene graph.

[N-Dekker]

Thanks @aylward I still have to have a closer look! A minor comment for now: if the 3 commits you added later are just fixes to your original proposal (rather than adjusting the existing code base of the ITK master branch), I would suggest to just squash them together with your first commit.

[aylward]

Will do!   Thanks!

[aylward]

Hi - thank you very much for the review!

One question - I thought it was standard practice to "squash and merge" as a last step, rather than doing force pushes as intermediate steps, since force pushes could be confusing/challenging for collaborators who have checkouts and might be working on the branch/pr. Additionally, during review it could be argued that it is good to see the conversation/process/steps rather than obscuring them? Again, with the final step being squash and merge.

Admittedly, my knowledge might be stale - I'm happy to learn and update my practices.

@N-Dekker, @matt and @dzenanz - what is recommended? Multiple force pushes during the review of a PR or doing a squash and merge as a single, final step?

Thanks! s

[dzenanz]

Additional commits make sense if the additions they contain would go into a separate commit anyway. Fixes to existing code are better done by amending + force pushing. We like to end up with logical commits, sometimes one, sometimes few, sometimes many. Having all PRs being squashed at the end into a single commit is what we try to avoid.

MONAI has a different approach - usually a single bit initial commit, followed by lots of small fix-oriented commits, all squashed into one commit upon merge.

[aylward]

Additional commits make sense if the additions they contain would go into a separate commit anyway. Fixes to existing code are better done by amending + force pushing. We like to end up with logical commits, sometimes one, sometimes few, sometimes many. Having all PRs being squashed at the end into a single commit is what we try to avoid.

MONAI has a different approach - usually a single bit initial commit, followed by lots of small fix-oriented commits, all squashed into one commit upon merge.

Thanks - happy to do it the ITK way for ITK :)

s

[thewtex]

Logically-coherent, self-contained commits that fully implement an idea and will pass tests when checked out and are quite helpful when reviewing history and backporting commits.

At the same time, we can keep this end result and also greatly lower the barrier to contributions by utilizing the "Squash and merge" feature of GitHub as @aylward suggests. Developers who know how to amend and force-push to craft their commits could still do so, but we could do better in not forcing contributors, especially new contributors, to learn this practice. Maintainers can squash and merge during release. These features are relatively new on GitHub and we could take advantage of them. We could try it and make a note about applying Squash and Merge when needed in our contributing docs in the Merge a Topic section

[thewtex]

We could try it and make a note about applying Squash and Merge when needed in our contributing docs in the Merge a Topic section

Patch here:

https://github.com/InsightSoftwareConsortium/ITK/pull/4750