Re: [boost] [review][qvm] QVM formal review report

Subject: Re: [boost] [review][qvm] QVM formal review report
From: Emil Dotchevski (emildotchevski_at_[hidden])
Date: 2016-01-06 22:39:11

I completed the refactoring based on the reviewers' feedback, except for
providing function-style syntax for swizzling, which requires more thought.
Note that this refactoring breaks almost any existing code that uses Boost
QVM, since a lot of functions now use more verbose names.

See http://zajo.github.io/boost-qvm/.

Thanks!
Emil

On Sun, Jan 3, 2016 at 10:11 AM, Adam Wulkiewicz <adam.wulkiewicz_at_[hidden]>
wrote:

> Dear Boost community,
>
> The review period for Emil Dotchevski's QVM library has concluded. I'd
> like to thank everyone who participated in the discussions, esspecially
> those who sent formal reviews.
>
> Only four votes were casted:
> - 1 vote for acceptance
>  * Rajaditya Mukherjee
> - 2 votes for conditional acceptance
>  * Rainer Deyke
>  * Oswin Krause
> - 1 vote for rejection
>  * Phil Endecott
>
> However more people commented on issues on the mailing list. Comments
> received during the course of the review were generally positive, with a
> number of technical issues identified that need to be addressed. The review
> identified no fundamental problems in the design or implementation of the
> library that would entail rejection. The final version of the library can
> be fully accepted once Emil has addressed the issues identified during the
> review. Therefore I'm happy to say:
>
>     Emil Dotchevski's Boost.QVM library is CONDITIONALLY ACCEPTED.
>
> Below is a summary of technical concerns:
>
> - The identifiers should be made more verbose. This is true both for
> function names as well as identifers containing "q"/"v"/"m" parts. The
> former should be replaced with full words, e.g. "transp" -> "transpose".
> The latter should be replaced at least with "qua"/"quat"/"vec"/"mat"
> respectively, so e.g. "v_traits" -> "vector_traits"/"vec_traits", "col_m"
> -> "col_mat", etc.
>
> - Many participants found the swizzling syntax using comma operator
> confusing or not good enough. There were many suggestions about the
> possible replacements:
>  * v%XY           - the old operator% syntax
>  * (v,XY)         - the comma operator syntax currently used by QVM
>  * (v,asXY)       - in order to limit potential conflicts with macros
>  * XY(v)          - function syntax, MOST PREFERED
>  * v.XY()         - probably the best but currently not possible (n4165,
> n4174)
>  * ref(v).XY()
>  * ref(v)[xy]
>  * swizzle(v, xy)
>  * ref(v)->xy
> The function syntax, i.e. XY(v), was the most prefered solution. Some of
> the participants appreciated the comma operator as well because of the
> similarity with shader languages syntax. The library could provide a few
> alternatives that could be picked by the user, e.g. both a comma and
> function syntax.
>
> - There were concerns regarding deduce_xx set of traits, in particular
> that traits taking two arguments (deduce_q2, deduce_v2 and deduce_m2) can
> cause ODR issues in a case when e.g. third-party libraries depending on QVM
> specialized these traits differently and those libraries were used
> together. There were no consensus if this is indeed a library issue or how
> this issue could be handled in general. Taking into account that the
> specialization of these traits is optional it'd be sufficient to describe
> the issue in the documentation and discourage the users from specializing
> those traits for types defined outside of their own code.
>
> - There was also a suggestion to use the same names that are already being
> used in other well-known libraries to make the interface more intuitive,
> e.g. mag() could be replaced by norm() (used in Eigen and Armadillo
> libraries), mag2() could be replaced by norm_squared() or norm_sqr(), etc.
> This would also make the latter more verbose. I'd let the decission how to
> deal with it to Emil.
>
> - Another suggestion was to replace read/write semantics with get/set
> semantics or at least to make the r/w/ir/iw identifiers more verbose. Here,
> I'd let the decission how to deal with it to Emil as well.
>
> - Aside from the issues described above, there were also suggestions that
> the scope of QVM library could/should be broadened to include
> functionalities from domains other than computer graphics, e.g. linear
> algebra, geometry, physics, robotics, etc. Oswin Krause requested that an
> consensus on the scope of the library should be made before the acceptance
> of the library. Phil Endecott expressed that the library is too small to be
> a standalone library. However as a Boost.Geometry developer I may say that
> the scopes, design rationales, internal structures, etc. of both libraries
> are different. AFAIK it's also true for Boost.Polygon. Furthermore, if my
> understanding is correct after reading various discussions the community
> finds the library useful even in the current state, some of the
> participants already used the library in their projects. Furthermore I'm
> sure that future users of QVM will suggest additions to the QVM library if
> they think it's necessary, so the scope will be broadened in a natural way.
> Though of course I encourage Emil to start a discussion with the community
> regarding this issue.
>
> - Regarding the documentation, various people suggested the following
> improvements:
>  * it should start with a "Quick Start" page showing how to use the QVM
> built-in types to do some high-school linear algebra,
>  * make it more obvious which functions are classified as "operations" and
> which are "view proxies",
>  * contain more examples of every function as well as some bigger
> real-life examples.
>
> Please let me know if you think something is wrong.
>
> Regards,
> Adam
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost