<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<title></title>
</head>
<body bgcolor="#ffffff" text="#000000">
Dmitry Kazakov wrote:<br>
<blockquote type="cite"><span class="Apple-style-span"
style="border-collapse: separate; color: rgb(0, 0, 0);
font-family: 'Times New Roman'; font-style: normal;
font-variant: normal; font-weight: normal; letter-spacing:
normal; line-height: normal; orphans: 2; text-indent: 0px;
text-transform: none; white-space: normal; widows: 2;
word-spacing: 0px; font-size: medium;">Ok. That's a really cool
patch, Silvio! You are awesome! =) But there are a few things to
fix.<br>
<br>
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.<br>
</span></blockquote>
Uhhh... yeaa.. i forgot the tests :(<br>
But... hmm... the test like they are now will not compile and I'm
afraid they never will :-/<br>
The changes I've done to the interface weren't done accidentally but
by purpose.<br>
It wouldn't be right/possible to fit the new KisCoordinatesConverter
class to the old class interface.<br>
I just fixed up the tests. They should compile now but won't pass.<br>
I only can change the tests so that they will pass (they will
definitively not pass in the current state).<br>
<br>
<blockquote type="cite"><span class="Apple-style-span"
style="border-collapse: separate; color: rgb(0, 0, 0);
font-family: 'Times New Roman'; font-style: normal;
font-variant: normal; font-weight: normal; letter-spacing:
normal; line-height: normal; orphans: 2; text-indent: 0px;
text-transform: none; white-space: normal; widows: 2;
word-spacing: 0px; font-size: medium;"> <br>
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:<br>
<br style="font-family: 'courier new',monospace;">
<span style="font-family: 'courier new',monospace;">
m_d->postprocessingTransform *=
QTransform::fromTranslate(-center.x(),-center.y());</span><br
style="font-family: 'courier new',monospace;">
<span style="font-family: 'courier new',monospace;">
m_d->postprocessingTransform *=
m_d->rotation.inverted();</span><br style="font-family:
'courier new',monospace;">
<span style="font-family: 'courier new',monospace;">
m_d->postprocessingTransform *= mirror;</span><br
style="font-family: 'courier new',monospace;">
<span style="font-family: 'courier new',monospace;">
m_d->postprocessingTransform *= m_d->rotation;</span><br
style="font-family: 'courier new',monospace;">
<span style="font-family: 'courier new',monospace;">
m_d->postprocessingTransform *=
QTransform::fromTranslate(center.x(),center.y());</span><br>
<br>
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?<br>
</span></blockquote>
I should have renamed m_postprocessingTransform because it really
has a different purpose then before.<br>
But I'm really sorry. I tried out other versions before and came to
the conclusion that there is IMHO no other<br>
way of doing it.<br>
I think you know that matrix multiplication is not cumulative so it
won't work (or just works to<br>
a certain limit) to have rotation, scaling and translation in
separate matrices, when you need to track the relative<br>
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.<br>
Of course I'm open for suggestions. If you know a way how to
preserve the separate matrices without loosing<br>
functionality i won't resist to change the code again :-)<br>
<br>
<blockquote type="cite"><span class="Apple-style-span"
style="border-collapse: separate; color: rgb(0, 0, 0);
font-family: 'Times New Roman'; font-style: normal;
font-variant: normal; font-weight: normal; letter-spacing:
normal; line-height: normal; orphans: 2; text-indent: 0px;
text-transform: none; white-space: normal; widows: 2;
word-spacing: 0px; font-size: medium;"> <br>
3) In kis_qpainter_canvas.cpp:122 you use .inverted() method
instead of using corresponding method directly. I guess, this is
a misprint =)<br>
</span></blockquote>
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).<br>
I changed it back :-)<br>
<br>
<blockquote type="cite"><span class="Apple-style-span"
style="border-collapse: separate; color: rgb(0, 0, 0);
font-family: 'Times New Roman'; font-style: normal;
font-variant: normal; font-weight: normal; letter-spacing:
normal; line-height: normal; orphans: 2; text-indent: 0px;
text-transform: none; white-space: normal; widows: 2;
word-spacing: 0px; font-size: medium;"> <br>
4) There is a bug:<br>
- open image<br>
- rotate the canvas a bit<br>
- try zooming in/out using the mouse wheel -- you'll see the
insane dancing of the canvas on a screen ;)<br>
</span></blockquote>
Ok.. yes :D i wasn't aware that this mouse wheel zooming feature
already exists.<br>
Now i really get what boud told me (i thought he made kind of a
feature request).<br>
Anyway... should work now like before. There was just some old code
that was causing oddness.<br>
I removed it since the KoCanvasControllerWidget class does the
calculations for the "zooming<br>
relative to a point" functionality already.<br>
<br>
Thank you for spending some time to look at my code :D<br>
I hope we can resolve this fast...<br>
<br>
<br>
</body>
</html>