[PATCH] Replace KisVector2D by Eigen

[Boudewijn Rempt]

On Friday 22 August 2008, jacob at math.jussieu.fr wrote:
> Hi,
>
> attached is a big patch on which I'd like your opinion. I hope that
> eventually you'll be OK to let me commit that, but it would be best if
> you could test it a bit before. I checked that the unit-tests succeed
> (except for two tests which have nothing to do with that) and took
> Krita for a basic test-drive.
>
> * Remove the KisVector2D and KisVector3D code (450 LOC) and instead
> make them be just typedef to Eigen2 vectors. These are vectors with
> qreal coefficients instead of double, as I think it is trendy to make
> things work on ARM embedded devices, but if you prefer we can keep
> double instead.
>
> [note: this is the second mini-vector-library that I remove from
> Krita, and there still are more (CImg)]

CImg is imported code: we shouldn't touch anything in there. I should, 
actually, look whether there's a new version of cimg and start using that, 
but since the filter is only barely useful, I haven't made time for that yet.

> * Port Krita to that change. Not a big deal, but there is at least one
> dangerous difference: KisVector2D::KisVector2D() used to initialize
> the coeffs to zero, which IMHO is a bad idea. No longer. When one
> really wants this behavior, one can now use the following Eigen2
> expression: KisVector2D::Zero().
>
> * Fix bogus fuzzy-comparisons of floating-point numbers as I found
> them. Sidenote: 99% of the times it is used, the DBL_EPSILON constant
> is used in a bogus way. I can elaborate on all that but don't want to
> bore you.

Well, I would like to learn!

> * Misc little fixes. For example, KisPaintInformationTest was
> generating "random" numbers by doing rand() / RAND_MAX  ;) Fixing this
> revealed that the exact comparisons then made between doubles were
> indeed failing, so I replaced them with fuzzy comparisons.
>
> *************
>
> Now a few lines to discuss the advantages of replacing the old KisVector2D:
>
> * its fuzzy comparisons were bogus
>
> * in Krita code you do a lot of nested expressions like:
>           result_vector = (vector1 + vector2) / 2;
> Thanks to expression templates, Eigen2 makes that compile to optimized
> code. By contrast, a 'classical' lib like the old KisVector2D
> generates very poor code here because it amounts to tmp1 = vector1 +
> vector2; tmp2 = tmp1/2; result_vector = tmp2; So there are a lot more
> load/store memory accesses.
>
> * Vectors of two double's are perfectly vectorizable since SSE and
> AltiVec are 128-bit. Eigen will vectorize all that automatically,
> provided SSE2 or AltiVec is enabled. Recall that at least the x86_64
> architecture defaults to SSE2 so at least on that arch you get
> immediately the benefit. On x86 you have to pass the -msse2 option to
> GCC but then you break compatibility with old (pentium 3) CPUs.
>
> * A large source of inefficiency remains the conversions to/from
> QPointF. This can be seen e.g. in kis_painter. As discussed on IRC, we
> cannot reliably use Eigen's ability to map a memory area as an Eigen
> expression here, because QPointF does not store its coords as a
> qreal[2] array.

Maybe it would be worth it to re-instate KisPoint (which was a float-based 
QPoint clone that got removed when QPointF appeared) and make it use an 
array. However, before doing that we should first investigate how often we 
would have to convert between QPointF and KisPointF.



-- 
Boudewijn Rempt 
http://www.valdyas.org/fading/index.cgi