lbalazscs / Pixelitor

A desktop image editor
https://pixelitor.sourceforge.io/
GNU General Public License v3.0
181 stars 70 forks source link

Magic Wand Optimized #352

Closed AnthonyyHL closed 6 months ago

AnthonyyHL commented 6 months ago

In this PR, I fixed the problems addressed in https://github.com/lbalazscs/Pixelitor/pull/338#issuecomment-1872507806. I tried to fix the fifth point, but tbh, i couldn't understand what you meant haha.

The most important change was the new Magic Wand algorithm. The shape of the selected area is obtained by walking through all pixels in a vertical line from the clicked pixel position. Then, for each pixel, it iterates horizontally to get a line segment of the pixels that can be selected. The final selected area is a group of all the line segments.

I await your opinion on the changes I made.

lbalazscs commented 6 months ago

This is a big improvement! In a test image, selecting the pixels of a spiral went down from 254 seconds to 10 seconds. It's also nice that the calculation is done on a background thread, and so the UI is not frozen. Yet I wonder if the processing time could be even shorter (ideally under 1 second) by not using Areas at all, but the algorithm I mentioned at https://github.com/lbalazscs/Pixelitor/pull/338#issuecomment-1872651338

Another thing that could improve performance a bit is the getColorAtEvent method in SelectionType. It uses BufferedImage.getRGB, which is not recommended for processing many pixels, because it is relatively slow. Typically, Pixelitor fetches the int array behind a BufferedImage using ImageUtils.getPixelArray, and then it accesses the ARGB int values directly, which is considerably faster (we can do this because in Pixelitor, images always use this ARGB-int encoding). Also notice how the isSimilar method in PaintBucketTool works on ints, while your colorWithinTolerance method in SelectionType requires creating two Color objects, which are immediately thrown away. You could simply reuse the isSimilar method instead (if you do, you can move it to the Colors utility class, and you can also rename it to colorWithinTolerance, I like both names)

Regarding point 5, it's unclear whether you're unsure about what I mean by a smoother shape or how I imagine smoothing with a PathIterator. I can explain it in more detail, if you are interested.

Anyway, I am merging this PR, because it's definitely an improvement. You can create a new PR if you want to continue contributing. And let me know if you have any questions!

lbalazscs commented 6 months ago

A Pixelitor user already used it (after building Pixelitor from the newest sources), but he also got a nullpointerexception.

Stack Trace: Cannot invoke "pixelitor.selection.SelectionBuilder.combineShapes(pixelitor.Composition)" because "this.selectionBuilder" is null pixelitor.tools.SelectionTool.notPolygonalDragFinished(SelectionTool.java:226) pixelitor.tools.SelectionTool.dragFinished(SelectionTool.java:192) pixelitor.tools.DragTool.mouseReleased(DragTool.java:109) pixelitor.tools.toolhandlers.CurrentToolHandler.mouseReleased(CurrentToolHandler.java:49) pixelitor.tools.toolhandlers.ToolHandler.handleMouseReleased(ToolHandler.java:62) pixelitor.tools.toolhandlers.ToolHandler.handleMouseReleased(ToolHandler.java:65) pixelitor.tools.toolhandlers.ToolHandlerChain.handleMouseReleased(ToolHandlerChain.java:90) pixelitor.tools.Tools$EventDispatcher.mouseReleased(Tools.java:234) pixelitor.gui.View.mouseReleased(View.java:288)

I can't reproduce the bug, and it's very strange, because in the previous line selectionBuilder is not null. I have two theories why this could happen: either the updateInProgressSelection call set this.selectionBuilder to null, or more than one background working threads are started and they interfere with each other somehow.