[
https://issues.apache.org/jira/browse/GEOMETRY-14?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16721985#comment-16721985
]
Gilles commented on GEOMETRY-14:
--------------------------------
{quote}plenty to review
{quote}
Indeed. Thanks for all that work!
Since we were talking about quaternions, I directed my attention to
{{QuaternionRotation}}.
First thing that struck me is the size of source file: More than 900 lines
looks a bit too much for a single class.
Then the name itself (literally) compounds high- and low-level functionality,
API and implementation; the class definitely represents the concept of
"rotation" (through its {{apply(Vector3D)}} and other methods that deal with
angles), but its API also almost completely duplicates the (Commons Numbers)
{{Quaternion}} API, although this "facet" of the class is never used outside
itself (which confirms that the rotation API is central, and that the class
should perhaps be renamed to... {{Rotation3D}}!).
To be more concrete, I've updated a branch named {{feature_GEOMETRY-14}}; it's
your PR, on which I've refactored the "slerp" functionality into its own
{{Slerp}} class.
Please have a look.
IMHO, this kind of change immediately makes the code leaner and clearer (and
provides for some performance improvement if several values of the
interpolation parameters are needed).
Additionally, the "slerp" code is now totally decoupled from the "rotation"
code.; and we can obviously wonder whether this should actually belong to the
{{commons-numbers-quaternion}} module...
I wouldn't be surprised if other such refactorings may prove interesting (e.g.
to provide/use functional interfaces).
WDYT?
Overall, it's really great that the library evolves rapidly, driven by your
actual use cases (I suppose).
However, not being a "power" user of the functionality, I can't hope to
achieve an exhaustive review, all by myself. :-{
> AffineTransform?D Classes
> -------------------------
>
> Key: GEOMETRY-14
> URL: https://issues.apache.org/jira/browse/GEOMETRY-14
> Project: Apache Commons Geometry
> Issue Type: New Feature
> Reporter: Matt Juntunen
> Priority: Major
> Labels: pull-request-available
> Fix For: 1.0
>
>
> We should create AffineTransform?D classes that implement matrix-based affine
> transforms. They should have simple methods for creating translations,
> rotations, and scaling and calculating the inverse.
>
> -Pull Request #1: [https://github.com/apache/commons-geometry/pull/14]-
> Pull Request #2: [https://github.com/apache/commons-geometry/pull/16]
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
