OK, before anyone says anything... I finally understood how to use KisPerspectiveTransformWorker & KisTransformWorker to rotate the fillDevice... I tried both of them and the rotation quality is waaay better, but there is no way we can use this, it's extremely slow. I've only played with it, the offsets are wrong, but I managed to correctly rotate the pattern in a somewhat seamless way (except for this offset problem). But as I've said... it is unbelievably slow, unusable. The QPainter + QBrush solution I submitted is way faster it seems... it's only the quality of the rotation is awful...

After thinking about it a bit I think that the best way would be to make a createLineIteratorNG & a KisWrappedLineIterator in the same idea as createHLineIteratorNG, but that can iterate taking in account a rotation & filter strategy. The filter might be too much, maybe bilinear is enough for this operation so just hard code it.

But with this in mind... I'm not really sure how to begin this. I understand the idea behind the iterators but implementing a new one is... an entirely different thing.

That is really bizar it is so slow. Did you try the usual speed-up thins, like maybe caching the transform somehow(by making a member variable) and only recreating it when the angle changes? That could make a huge difference.

In D6619#124101, @woltherav wrote:

That is really bizar it is so slow. Did you try the usual speed-up thins, like maybe caching the transform somehow(by making a member variable) and only recreating it when the angle changes? That could make a huge difference.

Wellll, I barely figured out how to use the Kis*Trasform* things... did not even cross my mind to do any caching :). I think that will improve it considerably. But the other thing is, I'm not entirely sure how KisPerspectiveTransformWorker manages the center of rotation. And it seems the only way to rotate around Z is to use the constructor with transform. I've tried adding a translation component to that but I couldn't get the patterns to align after rotation.

And the regular KisTransformWorker is even more slower so I don't want to touch that at all :).

I'll try and see if I can make it work with KisPerspectiveTransformWorker somehow.

OK, so I was reading about QPainter, QBrush & such, there's the setRenderHint/s method which I'm using and I think it produces slightly better results. Although I don't think it can be improved much more with this set-up. As I understand it these hints might or might not be used but on QImage it's more likely they're used. With the KisPerspectiveTransformWorker I just don't know how to align the brush dabs. Maybe something will come to me later. Now one problem with this QPainter, QBrush approach is that I think the applied value (here grayValue) is not the same as *iter->oldRawData() so I'd appreciate some help with this one.

Also as I've said... QPainter + QBrush does seem to be faster than the other options, although caching etc. is likely to improve that issue... if we can find a way to align the patterns after rotation. At the moment I don't see it...

I can't really help with the itterator problem because there's a huge chance the difference in look is because the Kis stuff is color managed and the QT stuff isn't. Which is why I am pushing so hard for your to use the KisPerspectiveTransform and friends.

I am not sure why the offset happens. Maybe @dkazakov or @jounip know?

@woltherav well, I know why the offset happens, it's because the rotation is about (0, 0) in the fillDevice reference system, so an adjustment needs to be made in order to center the pattern correctly after rotation, but I am not entirely sure how this fillDevice behaves.

The rotation needs to happen about the center of this fillDevice thing.

Then maybe use the other kisperspectivetransformworker constructor? because that has a transform center? (or look at the cpp class, so you can see how to make a qtransform use the transformation center)

The other constructor rotates around X & Y, not Z as we need here. What does fillPainter.fillRect(x - 1, y - 1, rect.width() + 2, rect.height() + 2, m_mask, m_maskBounds); do? Does it fill a rectangular region with top left corner at (x - 1, y - 1)? I assumed that but it can't be right, because I try applying the inverse translation and it just doesn't match.

So I was trying:

// after line 403 in the diff
double z = sqrt(rect.width() * rect.width() + rect.height() * rect.height());

// after 407
QTransform tf;
tf.translate(-x + 1 - (z + 2)/2.0, -y + 1 - (z + 2)/2.0);
tf.rotate(m_rotation);
tf.translate(x - 1 + (z + 2)/2.0, y - 1 + (z + 2)/2.0);
KisPerspectiveTransformWorker worker(fillDevice, tf, NULL);
worker.run();

And replace rect.width(), rect.height() with z before the for. But this doesn't work as expected...

In D6619#124196, @razvanr wrote:

The other constructor rotates around X & Y, not Z as we need here. What does fillPainter.fillRect(x - 1, y - 1, rect.width() + 2, rect.height() + 2, m_mask, m_maskBounds); do? Does it fill a rectangular region with top left corner at (x - 1, y - 1)? I assumed that but it can't be right, because I try applying the inverse translation and it just doesn't match.

So I was trying:

// after line 403 in the diff
double z = sqrt(rect.width() * rect.width() + rect.height() * rect.height());
 
// after 407
QTransform tf;
tf.translate(-x + 1 - (z + 2)/2.0, -y + 1 - (z + 2)/2.0);
tf.rotate(m_rotation);
tf.translate(x - 1 + (z + 2)/2.0, y - 1 + (z + 2)/2.0);
KisPerspectiveTransformWorker worker(fillDevice, tf, NULL);
worker.run();

And replace rect.width(), rect.height() with z before the for. But this doesn't work as expected...

Your assumption does seem to match the code.

However, if I understood your changes correctly, you are rotating around a center which is different for each dab. This will result in dabs that won't align with one another. I suggest you try changing the translation steps to move based on the center of the mask (m_maskBounds) rather than the dab (dab->bounds). The fill parameters would then also need to be adjusted so that that after rotation you'll end with the final dab area covered.

@jounip, I don't understand how this can be. Here x & y are defined based on m_maskBounds such that (say for x as example), x falls within 0 & m_maskBounds.width() - 1 (disregarding offsets for now). Then in order to translate around center point you have to translate to -x - 1 - half_of_fillRect_width, rotate, then translate back to original position. But I don't understand how this rotation works, I tried playing around with the translation/rotation & as far as I can tell the rotation happens around the top left corner of the fillRect region regardless of the translation. So basically what I observed is that translation has no effect.

The fill parameters would then also need to be adjusted so that that after rotation you'll end with the final dab area covered

Yes, that's why I defined z.

So from what I can tell there's no way to modify the pivot around where the rotation happens, at least not with KisPerspectiveTransformWorker. I know there's an option in the transform tool for changing the pivot location in the UI, but I couldn't find how that's implemented...

Sorry to add the further confusion, but I got confused myself. The geometry involved is actually a bit more complicated as the tiling of the pattern is not axis aligned to the canvas.

Here's a quick version of the necessary code:

void KisTextureProperties::apply(KisFixedPaintDeviceSP dab, const QPoint &offset, const KisPaintInformation & info)
{
    if (!m_enabled) return;

    KisPaintDeviceSP fillDevice = new KisPaintDevice(KoColorSpaceRegistry::instance()->alpha8());
    QRect rect = dab->bounds();
    qreal pressure = m_strengthOption.apply(info);
    quint8 *dabData = dab->data();

    KisFillPainter fillPainter(fillDevice);
    int x, y;

    if (m_rotation == 0 || m_maskBounds.width() > 512 || m_maskBounds.height() > 512) {
        x = offset.x() % m_maskBounds.width() - m_offsetX;
        y = offset.y() % m_maskBounds.height() - m_offsetY;

        fillPainter.fillRect(x - 1, y - 1, rect.width() + 2, rect.height() + 2, m_mask, m_maskBounds);
        fillPainter.end();
    } else {
        // Find the coordinates within the pattern which correspond to the dab location
        QTransform backwards;
        backwards.rotate(-m_rotation);

        QPointF uv = backwards.map(QPointF(offset));
        float u = fmod(uv.x(), m_maskBounds.width());
        float v = fmod(uv.y(), m_maskBounds.height());
        if (u < 0) u += m_maskBounds.width();
        if (v < 0) v += m_maskBounds.height();

        // Around said point, fill a region in fillDevice with the pattern
        // TODO: use a more fitting fill area
        double r = sqrt(rect.width() * rect.width() + rect.height() * rect.height());
        fillPainter.fillRect(u - r, v - r, 2*r, 2*r, m_mask, m_maskBounds);
        fillPainter.end();

        // Rotate fillDevice and move the point (u,v) to the origin
        QTransform tf;
        tf.rotate(m_rotation);
        tf.translate(-u, -v);
        KisPerspectiveTransformWorker worker(fillDevice, tf, NULL);
        worker.run();

        // Start copy to the dab from the origin
        x = y = 0;
    }

    KisHLineIteratorSP iter = fillDevice->createHLineIteratorNG(x, y, rect.width());
    for (int row = 0; row < rect.height(); ++row) {
        for (int col = 0; col < rect.width(); ++col) {
            if (m_texturingMode == MULTIPLY) {
                dab->colorSpace()->multiplyAlpha(dabData, quint8(*iter->oldRawData() * pressure), 1);
            } else {
                int pressureOffset = (1.0 - pressure) * 255;

                qint16 maskA = *iter->oldRawData() + pressureOffset;
                quint8 dabA = dab->colorSpace()->opacityU8(dabData);

                dabA = qMax(0, (qint16)dabA - maskA);
                dab->colorSpace()->setOpacity(dabData, dabA, 1);
            }
            iter->nextPixel();
            dabData += dab->pixelSize();
        }
        iter->nextRow();
    }
}

The code works, but the problem is performance, each time the dab leaves a mark on the canvas the rotation happens and it's super slow... if anyone can think of ways on how this can be sped up... this is the missing puzzle part.

the answer probably is caching.

@rempt, yeah but how? We need to use fillRect for each new position of the dab (offset) in the rotated mask space. I don't see how this can be cached...

Hi, @razvanr!

You can also optimize the call to fillPainter.fillRect(). In the start of the call generate data big enough and then just trickily adjust offset every time. That could be one solution.

Another solution is to implement that Gimp's maths for making the rotated texture seamless

@dkazakov hi there! The problem is you can't really have "a big enough" fillRect(), unless you limit the rotation angles to some very specific ones for creating seamless wraparound. Or if we assume that a person doesn't do a mark of more than X pixels, but that's too many assumptions... With the method here you never loose seamlessness and you don't need a huge fillRect() for that.

As for GIMP method I'm not familiar, can you point me in the right direction?

In D6619#125378, @razvanr wrote:

@dkazakov hi there! The problem is you can't really have "a big enough" fillRect(), unless you limit the rotation angles to some very specific ones for creating seamless wraparound. Or if we assume that a person doesn't do a mark of more than X pixels, but that's too many assumptions... With the method here you never loose seamlessness and you don't need a huge fillRect() for that.

I would need to think about that to know how to implement it. Basically, one needs to implement QBrush::transform() functionality for KisFillPainter. Right now you prepare a wrapped and rotated texture and then just paint it over the dab, but ideally, you can skip this preparation and just ask KisPainter to pick pixels from the correct transformed locations of the source unstransformed dab. You can actually try to reuse wrapped iterators for it... Though I don't know if it is doable. It needs some thinking...

As for GIMP method I'm not familiar, can you point me in the right direction?

There was some link on IRC with formulas for rotated seamless textures.

@dkazakov, actually I tried checking the qt source code to see how transform is implemented in QBrush... still trying to understand, but yes, I think this is by far the best approach. My initial test was using QPainter & QBrush and that didn't have any performance problems on my pc... (but of course that didn't include any filtering strategies)

I think I found where the fill is implemented in the qt source code: https://github.com/qt/qtbase/blob/e4c39d5e1e7ee8c2bba273e6d613ec519b7fa9c2/src/opengl/gl2paintengineex/qpaintengineex_opengl2.cpp#L745. It's... a lot of code there, don't even know if it's helpful to look at, I'll try to decipher it in the weekend...

So... it seems that all of the transformations are handled by OpenGL in QT. I'm guessing that's not the case in Krita? Fill operations are not performed using OpenGL are they? Browsing the fillRect code I couldn't find anything that would hint to this. So, in the end no matter where you put the rotation operation, you still need to do it for each dab which is going to be slow, even if it's going to be moved in the fill operation itself. There has to be a way to do this... :)

In D6619#125821, @razvanr wrote:

So... it seems that all of the transformations are handled by OpenGL in QT. I'm guessing that's not the case in Krita? Fill operations are not performed using OpenGL are they? Browsing the fillRect code I couldn't find anything that would hint to this. So, in the end no matter where you put the rotation operation, you still need to do it for each dab which is going to be slow, even if it's going to be moved in the fill operation itself. There has to be a way to do this... :)

iirc Krita doesn't use OpenGL for drawing to the image, in fact there is no GPU acceleration for that at all. OpenGL is only used when painting the canvas (to be presented to the user).

Right, that's what I thought. And with that in mind... I have no idea how to speed this up... I'm open to any suggestions :)

In D6619#125840, @razvanr wrote:

Right, that's what I thought. And with that in mind... I have no idea how to speed this up... I'm open to any suggestions :)

Out of curiosity, I tried sampling the pattern directly like Dmitry suggested. It still looks about twice as slow as with an unrotated pattern. Code: https://paste.kde.org/pyfdgkgxc/ir0woj

Another option would be to cache a rotated version of the pattern, but that will still need interpolation or result in slight misalignment from rounding each tiled destination to whole pixels. It's rather looks like a classic case of pick two between speed, quality and memory footprint.

@jounip, seems the paste you added is password protected.

As for the method, it's exactly what I expected, cause in the end it doesn't matter if the rotation is in the apply or in the fillRect it's going to be called each time and the rotation is the one giving performance issues. I don't agree with rotating just once and caching the mask, cause the misalignment can result in very bad quality. There has to be a way to use QPainter & QBrush so that the operation is the same as the operation with the KisPaintDevice. QPainter & QBrush is what I used in the first attempt and that was fairly fast, I didn't see any noticeable slowdowns. Check out the first diff I uploaded. The only problem with that is the operation itself, I used the grayValue, but I'm sure that's incorrect. I just don't know how the correct operations are performed int he KisPaintDevice.

In D6619#125907, @razvanr wrote:

@jounip, seems the paste you added is password protected.

Oops. Sorry about that. Should be public now.

Edit: I also just came across KisWrappedRandomAccessor. It would be preferable to use that rather than adding the new accessor function in the pasted code.

In D6619#125938, @jounip wrote:

Edit: I also just came across KisWrappedRandomAccessor. It would be preferable to use that rather than adding the new accessor function in the pasted code.

Yes, this accessor is used in the wraparound mode, so it can be reused.

About the speed being twice slower, it doesn't seem to be too bad. Probably, we could implement caching for the predefined angles using this algorithm link, and for all the other angles (which make the cache be, say, more than 2000 pixels in size) use this slow implementation.

In D6619#126994, @dkazakov wrote:

In D6619#125938, @jounip wrote:

Edit: I also just came across KisWrappedRandomAccessor. It would be preferable to use that rather than adding the new accessor function in the pasted code.

Yes, this accessor is used in the wraparound mode, so it can be reused.

About the speed being twice slower, it doesn't seem to be too bad. Probably, we could implement caching for the predefined angles using this algorithm link, and for all the other angles (which make the cache be, say, more than 2000 pixels in size) use this slow implementation.

Well, I tried the solution on my laptop which isn't really that slow (it's mx14 alienware, i7, nvidia dedicated) and this is so slow it's not usable, I mean you can paint with it sure, but it would be super frustrating. See example here https://twitter.com/razvanc_r/status/886507507593736193. Is it really impossible to use QPainter & QBrush for this? I don't have an example vid on that, but with that solution I didn't see any noticeable slowdowns. Only problem is I don't know the proper way to multiply/subtract with QPainter. Oh and... we need a way to smooth the rotation, bilinear interpolation or something. I saw you can set a smooth hint on the brush, which (I might be wrong but) looks like it might improve the rotation quality.

So, out of curioisty I build this myself, and yes, it is too slow(as well as having a weird kind of gradient issue added)

However, I also managed to track down the rotation code that QT uses locally:

https://github.com/qt/qtbase/blob/e4c39d5e1e7ee8c2bba273e6d613ec519b7fa9c2/src/gui/painting/qpaintengine_raster.cpp#L2227

However, it seems to use something that seems to be templatehell:
https://github.com/qt/qtbase/blob/e4c39d5e1e7ee8c2bba273e6d613ec519b7fa9c2/src/gui/painting/qmemrotate.cpp

I don't understand said code, but I figured it'd be good to post the links here.

I've been thinking about this lately and I think we can implement this another way that wouldn't lower the performance. The problem with this implementation is that the pattern gets rotated for each dab placement. So there's two ways I can think of improving this, either apply the rotation and store it somewhere else outside of the paint operation or rotate it once at the beginning of the paint stroke and cache it somehow.

I'm not sure where this could be implemented outside of the stoke placement itself because I'm still not familiar with the code, but I'm sure some of you have an idea :).

The aliasing problem is a separate thing then.

Hi, @razvanr!

In D6619#382920, @razvanr wrote:

I've been thinking about this lately and I think we can implement this another way that wouldn't lower the performance. The problem with this implementation is that the pattern gets rotated for each dab placement. So there's two ways I can think of improving this, either apply the rotation and store it somewhere else outside of the paint operation or rotate it once at the beginning of the paint stroke and cache it somehow.

Yes, we could do some caching. One of the options is to use KisCanvasResourceProvider to cache this thing. Just implement one more (non-storable) resource type for that. But as with any caching, we should answer the following questions first:

1. When to generate the cache? If we generate right at the start of the first stroke, then the user will see some delay. As an option, we could do some background asynchronous action for generation (KisSpontaneousJob). This job could be executed only after the user has closed the brush editor.
2. How to link a cache for a particular configuration of the pattern to the cached item? Can multiple presets with the same configuration of the patterns link to the same cache item?
3. When to delete the cache item? When a preset changed? Or only when a pattern settings are changed? What happens if the user didn't use this preset for an hour, should we still keep it in the memory?

@dkazakov I've been thinking about the options as well.

I think the most reasonable option is to do the transformation at the brush editor level as you mentioned. And just now I've been considering that since the pattern is embedded in the brush preset as well as a blob, then why not store it like that? So let me ask you this: what happens with the pattern when you change some settings in the Brush Editor > Pattern > Options tab? If the transformation (say scale) happens at the brush dab/stroke level, then this needs to be reworked for the entire brush settings in my opinion. It's most likely impacting the performance of the brush stroke considerably. So whatever decision we make for the rotation option it should be generic enough and used for all settings and implemented outside of the brush dab level. So with that in mind:

1. the better option is to make the change once the brush editor closes
2. I don't think other presets should link to the cached pattern since these options are per paintoppreset, not per pattern so we need a system to perhaps either re-generate the pattern when switching the paintoppreset or store it some other way
3. considering the above, then it makes sense to reset/recreate the pattern upon paintoppreset change. I don't know exactly how to answer about this time-limit. If we would recreate the cache upon paintoppreset change then I'd say the cache shouldn't have a time limit, it should be kept for as long as krita is open. In this scenario we've been talking about one cache element for the entire krita lifetime so it shouldn't make much different to memory usage.