Add feather effect#953
Conversation
cameronwhite
left a comment
There was a problem hiding this comment.
Thanks for contributing! Left a few suggestions in the comments
| if (sx < 0 || sx >= src_width || sy < 0 || sy >= src_height) | ||
| continue; | ||
|
|
||
| if (src.GetColorBgra (src_data, src_width, pixel).A != 0) |
There was a problem hiding this comment.
Should this be comparing against the transparency threshold?
| continue; | ||
|
|
||
| if (src.GetColorBgra (src_data, src_width, pixel).A != 0) | ||
| borderPixels.Add (potentialBorderPixel); |
There was a problem hiding this comment.
I think the same pixel could be added to the list several times potentially?
| // For each pixel, lower alpha based off distance to border pixel | ||
| foreach (var borderPixel in borderPixels) { | ||
| for (int y = borderPixel.Y - radius; y <= borderPixel.Y + radius; ++y) { | ||
| for (int x = borderPixel.X - radius; x <= borderPixel.X + radius; x++) { |
There was a problem hiding this comment.
I think the issue with modifying the image outside the selection is because these coordinates aren't clamped to the regions being rendered (or the image size even?)
There was a problem hiding this comment.
The issue I am having, is that the border pixels have to affect pixels outside their roi. I can't make the system work backwards (each pixel modifies itself based on nearby border pixels) because of threading (Pixels may not be aware of some border pixels.) I dont think it's possible to see all the rois at once.
That being said, yeah i should be clamping to at least the border size...
There was a problem hiding this comment.
I think if you define IsTileable to be false, then you'll get a single region for the whole image (it's up to you to then do any multithreading internally)
That could be more suitable here if you want to do a first pass to find border pixels and then a second pass to adjust alpha, or something like that?
| [Caption ("Radius"), MinimumValue (1), MaximumValue (100)] | ||
| public int Radius { get; set; } = 6; | ||
|
|
||
| [Caption ("Transparency Threshold"), MinimumValue (0), MaximumValue (255)] |
There was a problem hiding this comment.
Minor suggestion: Tolerance rather than Threshold would be a closer match for other terminology like the Magic Wand tool
|
One other note is that you can also add tests for the effect in https://github.com/PintaProject/Pinta/blob/master/tests/Pinta.Effects.Tests/EffectsTest.cs |
I can, but because feather replies on transparency, the output is identical to the test input image. |
Right, good point :) Feel free to adjust that |
|
Should be good now. |
|
|
||
| public override void Render (ImageSurface src, ImageSurface dest, ReadOnlySpan<RectangleI> rois) | ||
| { | ||
| foreach (var roi in rois) { |
There was a problem hiding this comment.
With IsTileable = false, I think the effect should only ever be invoked with a single region, so it would be slightly simpler to instead override the Render signature that takes a single rectangle (similar to the Dithering effect)
| // crashes on test if try-catch not implemented | ||
| int threadCount = 0; | ||
| try { | ||
| threadCount = PintaCore.System.RenderThreads; |
There was a problem hiding this comment.
The proper fix here would be to add an ISystemService similar to the IChromeService , so the effect doesn't need to depend on having all of PintaCore initialized and the unit tests can pass in a mock implementation
| } catch { | ||
| threadCount = 1; | ||
| } | ||
| var slaves = new Thread[threadCount]; |
There was a problem hiding this comment.
Using Parallel.For would likely be a lot simpler than manually managing threads (an example is in the Paint Bucket tool) - the code from the LivePreviewManager should probably be refactored to do this in the future too
For limiting the number of threads, something like https://stackoverflow.com/a/15931412 should work
| int pixel_index = py * src_width + px; | ||
| float mult = distance / radius; | ||
| byte alpha = (byte) (src_data[pixel_index].A * mult); | ||
| if (alpha < dst_data[pixel_index].A) |
There was a problem hiding this comment.
I'm not sure this is thread safe - multiple threads could be modifying the same pixels simultaneously?
There was a problem hiding this comment.
I checked. Unfortunately, you are right.
I'll rewrite it to work on a per-pixel basis. Was avoiding that because it's more expensive and requires an extra pass, but I guess it's inevitable.
|
Implemented feedback. Should be thread safe now. |
cameronwhite
left a comment
There was a problem hiding this comment.
Looks good! I just had one question in the comments about the loop range
| // Color in any pixel that the stencil says we need to fill | ||
| // First pass | ||
| // Clean up dest, then collect all border pixels | ||
| Parallel.For (0, bottom, new ParallelOptions { MaxDegreeOfParallelism = threads }, y => { |
There was a problem hiding this comment.
Should this loop start from the top of the rectangle, rather than the top of the image?
Same for the second parallel loop too
There was a problem hiding this comment.
Oops... Good catch!
| int src_width = src.Width; | ||
| var dst_data = dest.GetPixelData (); | ||
|
|
||
| var relevantBorderPixels = borderPixels.Where (borderPixel => borderPixel.Y > py - radius && borderPixel.Y < py + radius).ToArray (); |
There was a problem hiding this comment.
A future improvement for another PR would be to set up an acceleration structure for the closest border pixel lookup (e.g. KD tree), to avoid going through all of the border pixels for every row. I think we have a spatial hashing implementation in the shape tools as well
|
Looks good, thanks for your contribution! |
Fixes #886
Adds feather tool to Effects > Stylize (Considered blur, but I think this is more fitting). This softens the edges of an image without spilling blur out of the image or blurring the contents of the image.
Radius adjusts the strength of the effect, Transparency Threshold is used to determine the "outline" of the object. Requires object be surrounded by transparency to function.
Example with a test image.
Only known issue I am aware of is that the effect will spill over outside selection in preview, however it does not spill over after application. Unsure how to fix this.