Branch: krita-canvasrotation-silvioheinrich

[Silvio Heinrich]

Dmitry Kazakov wrote:
> Ok. That's a really cool patch, Silvio! You are awesome! =) But there 
> are a few things to fix.
>
> 1) You didn't compile the tests. There are tests for 
> KisCoordinatesConverter and KisPrescaledProjection. They test public 
> interface of the converter. They all should pass before we can say the 
> patch is safe for the master. To activate the tests you need to set 
> KDE4_BUILD_TESTS=TRUE in cmake.
Uhhh... yeaa.. i forgot the tests :(
But... hmm... the test like they are now will not compile and I'm afraid 
they never will :-/
The changes I've done to the interface weren't done accidentally but by 
purpose.
It wouldn't be right/possible to fit the new KisCoordinatesConverter 
class to the old class interface.
I just fixed up the tests. They should compile now but won't pass.
I only can change the tests so that they will pass (they will 
definitively not pass in the current state).

>
> 2) I don't really approve the way you use m_postprocessingTransform. 
> You aggregated all the image transformations inside this structure, 
> that is not really good, because this transform does not represent any 
> real-world coordinate system (c.s.) anymore (in your patch). In the 
> previous version every QTransform represented a conversion between two 
> distinct coordinate systems (out of 6 we have in Krita). And 
> m_postprocessingTransform was only in charge of canvas effects like 
> rotation and mirroring. You can read about them in [0] and see a 
> picture in attachment. Now you are forced to use such a code to change 
> relative position of one c.s. against another one:
>
>     m_d->postprocessingTransform *= 
> QTransform::fromTranslate(-center.x(),-center.y());
>     m_d->postprocessingTransform *= m_d->rotation.inverted();
>     m_d->postprocessingTransform *= mirror;
>     m_d->postprocessingTransform *= m_d->rotation;
>     m_d->postprocessingTransform *= 
> QTransform::fromTranslate(center.x(),center.y());
>
> I would really like you split these transformation into the distinct 
> ones (representing distinct c.s.'s). Or, maybe, you had some good 
> reason to merge them?
I should have renamed m_postprocessingTransform because it really has a 
different purpose then before.
But I'm really sorry. I tried out other versions before and came to the 
conclusion that there is IMHO no other
way of doing it.
I think you know that matrix multiplication is not cumulative so it 
won't work (or just works to
a certain limit) to have rotation, scaling and translation in separate 
matrices, when you need to track the relative
change of one coordinate system, and then multiply them together in the 
end. For further work i need that the krita canvas is able to rotate, 
zoom and mirror the image around an arbitrary point and this flexibility 
only comes with the code above.
Of course I'm open for suggestions. If you know a way how to preserve 
the separate matrices without loosing
functionality i won't resist to change the code again :-)

>
> 3) In kis_qpainter_canvas.cpp:122 you use .inverted() method instead 
> of using corresponding method directly. I guess, this is a misprint =)
well.. no. i changed the viewportToWidgetTransform() method to 
widgetToViewportTransform() but now as you mentioned i realized that 
this was unnecessary (and will cause more problems with the unit tests).
I changed it back :-)

>
> 4) There is a bug:
> - open image
> - rotate the canvas a bit
> - try zooming in/out using the mouse wheel -- you'll see the insane 
> dancing of the canvas on a screen ;)
Ok.. yes :D i wasn't aware that this mouse wheel zooming feature already 
exists.
Now i really get what boud told me (i thought he made kind of a feature 
request).
Anyway... should work now like before. There was just some old code that 
was causing oddness.
I removed it since the KoCanvasControllerWidget class does the 
calculations for the "zooming
relative to a point" functionality already.

Thank you for spending some time to look at my code :D
I hope we can resolve this fast...


-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kimageshop/attachments/20110204/6eab679a/attachment.htm