I don't feel that the changes to kis_import_qmic_processing_visitor.cpp and kis_qmic_simple_convertor.cpp should be considered valid.

My changes to those files compensate for the layer's position on the canvas.
When making the patch, I had not compared the code from the current (gmic-qt) plug-in to the old plug-in. Once I did so, I noticed that the old plug-in had the same lines.
This leads me to believe that the real problem is elsewhere and that I was fixing the wrong thing.

I should elaborate on the two bugs that the patch brings, because my summary was rather unclear:

• A tile with a negative position offset will appear to have the negative value set to 0 after applying a G'MIC filter. This can be reproduced by
  1. Moving a layer to the right side of the canvas
  2. Selecting a portion of it
  3. Moving the selected portion to the left
  4. Applying a G'MIC filter. The result will be that the moved portion becomes aligned to the left side of the layer.
• Using G'MIC on a layer with a transform mask does not use the un-transformed layer. This can make it impossible to use G'MIC properly while a transform mask is enabled. [EDIT 2017-07-08 15:07 UTC This also happens without any patches applied.]

Both cases work fine when using the old plug-in.

I suggest trying these on the file included in my bug report. I can provide better test files if desired.

The parts of this patch that I think might be okay are:

if (messageMap.values("command").first() == "gmic_qt_get_image_size") {
    QRect layerSize = m_view->activeNode()->projection()->nonDefaultPixelArea();
    ba = QByteArray::number(layerSize.width()) + "," + QByteArray::number(layerSize.height());
}

Currently, Krita appears to send G'MIC the dimensions of the full image, not the dimensions of what G'MIC will be operating on. This is the cause of #4 in my report.

I don't know if nonDefaultPixelArea() should be preferred over exactBounds(). The preview in the old plug-in gives a tighter fit, suggesting the usage of exactBounds() instead.

If nonDefaultPixelArea() is preferred, then

if (node->paintDevice()) {
   qWarning() << "Converting node" << node->name() << node->projection()->nonDefaultPixelArea();
   QRect cropRect = node->projection()->nonDefaultPixelArea();

is probably okay too, and actually required for the patch as a whole (otherwise, the resulting image can be smaller than the destination, and the original image will be visible underneath).

I also don't know if I should be checking the validity of any of the pointers that lead up to the calls of nonDefaultPixelArea().

If those are fine, then all my changes to QMic.cpp might be okay, but I wonder if they still need fixes for the issue with transform masks.

Hi, @nicholasl!

In D6431#123057, @nicholasl wrote:

I should elaborate on the two bugs that the patch brings, because my summary was rather unclear:

• A tile with a negative position offset will appear to have the negative value set to 0 after applying a G'MIC filter. This can be reproduced by
  1. Moving a layer to the right side of the canvas
  2. Selecting a portion of it
  3. Moving the selected portion to the left
  4. Applying a G'MIC filter. The result will be that the moved portion becomes aligned to the left side of the layer.

From the mere reading of your explanation it looks as if GMic tries to read data directly from the data manager, because the MoveTool introduces (x,y) offset for a layer and, therefore, paint device, which is then handled by the methods of KisPaintDevice and iterators. If one uses device->readBytes/readPlanarBytes(), then everything should be fine.

• Using G'MIC on a layer with a transform mask does not use the un-transformed layer. This can make it impossible to use G'MIC properly while a transform mask is enabled. [EDIT 2017-07-08 15:07 UTC This also happens without any patches applied.]

That might be related to the fact that GMic reads from layer->projection() instead of layer->paintDevice(), which it should read from.

I don't know if nonDefaultPixelArea() should be preferred over exactBounds(). The preview in the old plug-in gives a tighter fit, suggesting the usage of exactBounds() instead.

By default to get a region to process all the code should use device->exactBounds(). nonDefaultPixelArea() should only be used in few cases when the code handles device->setDefaultPixel() manually or uses some optimization.

For the rest of GMic-related questions I don't have enough competence I'm afraid :)

In D6431#123202, @dkazakov wrote:

From the mere reading of your explanation it looks as if GMic tries to read data directly from the data manager, because the MoveTool introduces (x,y) offset for a layer and, therefore, paint device, which is then handled by the methods of KisPaintDevice and iterators. If one uses device->readBytes/readPlanarBytes(), then everything should be fine.

As long as the chunk involving KisAlgebra2D is in place, passing image data from Krita to G'MIC seems to work okay (except for when there are transform masks, of course).

That might be related to the fact that GMic reads from layer->projection() instead of layer->paintDevice(), which it should read from.

It appears to be using node->paintDevice() already, and there are no mentions of projection() in the plug-in directory beyond my calls to nonDefaultPixelArea().
The code which does the actual reading of pixels (KisQmicSimpleConvertor::convertToGmicImageFast()) has not changed in a significant way between the old and new versions of the plug-in.

Going from G'MIC to Krita is where it seems to get messy. Unless I adjust the destination position, the result will always be placed at 0,0 on the canvas.
The code which does the actual setting of pixels (KisQmicSimpleConvertor::convertFromGmicFast()) has not changed at all between the old and new versions of the plug-in.
The functions which can call that, KisImportQmicProcessingVisitor::gmicImageToPaintDevice() and KisImportQmicProcessingVisitor::visitNodeWithPaintDevice(), also don't appear significantly changed.

By default to get a region to process all the code should use device->exactBounds(). nonDefaultPixelArea() should only be used in few cases when the code handles device->setDefaultPixel() manually or uses some optimization.

I'll switch to exactBounds(), then. As I mentioned, using exactBounds() makes it work a little worse, but I imagine that fixing whatever is causing the resulting image to be placed at 0,0 would also fix that.

For the rest of GMic-related questions I don't have enough competence I'm afraid :)

No problem; thanks for any help you can provide. :)

It seems to work identically to the old plug-in now.

When there was no selection, the old plug-in would read and write pixels within the rectangle (0,0 canvasWidth x canvasHeight).
The new plug-in was still set up to do this, except it would send the rectangle (0,0 layerWidth x layerHeight) to G'MIC.

When there was a selection in the old plug-in, it would read and write within the selection rectangle.
The new plug-in would read using (0,0 canvasWidth x canvasHeight), then write inside the selection rectangle.

This revision should reinstate the old behavior.

I have noticed a couple of bugs, but they were also in the old plug-in:

• Off-canvas portions of the layer are not affected by filters (I already mentioned this one in my bug report)
• Only the global selection is effective -- local selections are ignored

If this patch is pushable, I'll suppose I'll close my report and make separate ones for these.
Also, if I can push it to the stable branch as well, how should I handle the lack of mapToRect()?

In D6431#123570, @dkazakov wrote:

Sounds strange, because KisTransformationMask doesn't write anywhere except the projection() device of the layer

Somehow, I thought that I was seeing the transformed layer, but I believe I was mistaken. :)

On an aside, I didn't touch this part of the code, but I wonder if it could ever cause a problem:

else if (messageMap.values("command").first() == "gmic_qt_get_cropped_images") {
    // Parse the message, create the shared memory segments, and create a new message to send back and waid for ack
    QRectF cropRect = m_view->image()->bounds();
    if (!messageMap.contains("croprect") || messageMap.values("croprect").first().split(',', QString::SkipEmptyParts).size() != 4) {
        qWarning() << "gmic-qt didn't send a croprect or not a valid croprect";
    }
    else {
        QStringList cr = messageMap.values("croprect").first().split(',', QString::SkipEmptyParts);
        cropRect.setX(cr[0].toFloat());
        cropRect.setY(cr[1].toFloat());
        cropRect.setWidth(cr[2].toFloat());
        cropRect.setHeight(cr[3].toFloat());
    }
    if (!prepareCroppedImages(&ba, cropRect, mode)) {
        qWarning() << "Failed to prepare images for gmic-qt";
    }
}

The cropRect from G'MIC is a normalized rectangle. m_view->image()->bounds() is (0,0 canvasWidth x canvasHeight) (hopefully, or else my patch is wrong).
From how it's written, I imagine the intended default was (0,0 1.0x1.0).

The cropRect from G'MIC is a normalized rectangle. m_view->image()->bounds() is (0,0 canvasWidth x canvasHeight) (hopefully, or else my patch is wrong).
From how it's written, I imagine the intended default was (0,0 1.0x1.0).

What we get from gmic was, afair, a rectangle normalized to 0,0 1.0x1.0 -- so in prepareCroppedImages, that was multiplied with the dimensions of the layer.

In D6431#124086, @rempt wrote:

The cropRect from G'MIC is a normalized rectangle. m_view->image()->bounds() is (0,0 canvasWidth x canvasHeight) (hopefully, or else my patch is wrong).
From how it's written, I imagine the intended default was (0,0 1.0x1.0).

What we get from gmic was, afair, a rectangle normalized to 0,0 1.0x1.0 -- so in prepareCroppedImages, that was multiplied with the dimensions of the layer.

That is correct.
Any potential problem here would be from the error case, where we will no longer have a normalized rectangle.

Let's say we have a 1024x768 canvas.
We initialize cropRect using m_view->image()->bounds(), giving us (0,0 1024.0x768.0), then, if G'MIC sent a proper value for it, we modify cropRect to match what it sent. Since G'MIC sends a normalized rectangle, everything works fine.
If G'MIC "didn't send a croprect or not a valid croprect", then we are left with the rectangle from bounds(), which I can't imagine being what we want.

If we use (0,0 1.0x1.0) as the default instead, we will send the full canvas size to G'MIC.