Bug 392191 - Height Map to Normal Map creates a line at the top/bottom edge.
Summary: Height Map to Normal Map creates a line at the top/bottom edge.
Status: RESOLVED FIXED
Alias: None
Product: krita
Classification: Applications
Component: Filters (show other bugs)
Version: 4.0
Platform: unspecified All
: NOR normal
Target Milestone: ---
Assignee: Krita Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-22 19:57 UTC by sidartalei
Modified: 2018-04-03 11:47 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
The canvas in wrap mode, the lines become apparent. (1.50 KB, image/png)
2018-03-22 19:57 UTC, sidartalei
Details

Note You need to log in before you can comment on or make changes to this bug.
Description sidartalei 2018-03-22 19:57:04 UTC
Created attachment 111558 [details]
The canvas in wrap mode, the lines become apparent.

The Height Map to Normal Map filter creates height information on the top and bottom of the image.
Comment 1 wolthera 2018-03-22 19:59:04 UTC
Yes, this is as far as I understand a problem with the convolution painter itself. I pass the wraparound variable correctly, as far as I know.
Comment 2 Dennis Ranke 2018-03-23 11:15:59 UTC
I did a read through the code and I /think/ I identified two issues that relate to this bug:

1. In KisEdgeDetectionKernel::convertToNormalMap there is this code to apply the horizontal and vertical convolution kernels:

horizPainterLR.applyMatrix(kernelHorizLeftRight, device,
                           srcTopLeft - QPoint(0, ceil(horizontalCenter)),
                           srcTopLeft - QPoint(0, ceil(horizontalCenter)),
                           rect.size() + QSize(0, 2 * ceil(horizontalCenter)), BORDER_REPEAT);

and

verticalPainterTB.applyMatrix(kernelVerticalTopBottom, device,
                              srcTopLeft - QPoint(0, ceil(verticalCenter)),
                              srcTopLeft - QPoint(0, ceil(verticalCenter)),
                              rect.size() + QSize(0, 2 * ceil(verticalCenter)), BORDER_REPEAT);

If you look at the QPoints and the QSize, you see that in both cases the x is set to zero and the y to some value based on the horizontal/vertical center. I guess that in one of the cases, probably the vertical one, the two vector components should be flipped around.

2. The definition of BORDER_REPEAT which you pass into applyMatrix is defined as:

BORDER_REPEAT = 1  // Use the border for the missing pixels

Which is actually a clamp, not a wrap-around. So a wrapping normal map will have some artifacts on the borders. Ideally, the filter would gain an option to select a newly coded BORDER_WRAP mode, although that's really more of a feature request rather than a bug.
Comment 3 Dennis Ranke 2018-03-23 11:21:37 UTC
Ok, I guessed wrong, it's actually the horizontal applyMatrix that is wrong:

horizPainterLR.applyMatrix(kernelHorizLeftRight, device,
                           srcTopLeft - QPoint(ceil(horizontalCenter), 0),
                           srcTopLeft - QPoint(ceil(horizontalCenter), 0),
                           rect.size() + QSize(2 * ceil(horizontalCenter), 0), BORDER_REPEAT);

gets rid of the line at the top and bottom edges for me.
Comment 4 Halla Rempt 2018-03-28 14:08:56 UTC
Git commit bc30d50e80024d7b2352fa0b3112ecff5e9c36aa by Boudewijn Rempt.
Committed on 28/03/2018 at 14:08.
Pushed by rempt into branch 'master'.

Fix height map to normal map in wraparound mode
patch by Dennis Ranke. Thanks!

M  +3    -3    libs/image/kis_edge_detection_kernel.cpp

https://commits.kde.org/krita/bc30d50e80024d7b2352fa0b3112ecff5e9c36aa
Comment 5 Halla Rempt 2018-04-03 11:47:06 UTC
Git commit a0e2a23f0e7ab1054b9fd49cad29c3c58590cacc by Boudewijn Rempt.
Committed on 03/04/2018 at 11:18.
Pushed by rempt into branch 'krita/4.0'.

Fix height map to normal map in wraparound mode
patch by Dennis Ranke. Thanks!

(cherry picked from commit bc30d50e80024d7b2352fa0b3112ecff5e9c36aa)

M  +3    -3    libs/image/kis_edge_detection_kernel.cpp

https://commits.kde.org/krita/a0e2a23f0e7ab1054b9fd49cad29c3c58590cacc