search

mitchcurtis / slate

Pixel Art Editor
GNU General Public License v3.0
1.07k stars 103 forks source link

Fix pen tool behaviour by using line tool for all drawing #92

Closed not-surt closed 5 years ago

not-surt commented 5 years ago

Fixes broken stroke in freehand, wrong offset from start of line, radiating held line drawing, use round pen to get clean angle transitions.

Image canvas only so far. For tile canvas would be better to merge both into single class to remove code duplication, treating image canvas as a tile canvas with one single full image sized tile.

mitchcurtis commented 5 years ago

Hello again! I haven't looked at the changes in detail yet, but I have tested them out and like the general approach.

Fixes broken stroke in freehand, wrong offset from start of line,

I guess this is the issue where the start of the line starts with the top left at the centre instead of the top left.. if that makes sense? I think that with this patch, #7 is completely fixed, so thank you! :)

radiating held line drawing

Not sure what this means.

use round pen to get clean angle transitions.

This is really cool; it's very consistent now. My only issue with it is that it changes behaviour with no way to opt out (no way to use a square brush). I'd like to allow users to switch between square and round (with a tool button on the tool bar, for example).

Image canvas only so far. For tile canvas would be better to merge both into single class to remove code duplication, treating image canvas as a tile canvas with one single full image sized tile.

This is an interesting idea... I would need to think about how this would work.

There are some test failures that would need to be fixed before this can be merged:

16:45:22: Starting /Users/mitch/dev/slate-qt5_slate_fw-Debug/qtc_qt5_slate_fw_Debug/test-app.99bdad93/test-app.app/Contents/MacOS/test-app...
QML debugging is enabled. Only use this in a safe environment.
2018-11-11 16:45:23.085 test-app[6212:136956] ApplePersistenceIgnoreState: Existing state will not be touched. New state will be written to (null)
********* Start testing of tst_App *********
Config: Using QtTest library 5.13.0, Qt 5.13.0 (x86_64-little_endian-lp64 shared (dynamic) debug build; by Clang 10.0.0 (clang-1000.11.45.2) (Apple))
PASS   : tst_App::initTestCase()
PASS   : tst_App::newProjectWithNewTileset()
PASS   : tst_App::repeatedNewProject(TilesetType, ImageType)
PASS   : tst_App::repeatedNewProject(ImageType, LayeredImageType)
PASS   : tst_App::repeatedNewProject(LayeredImageType, TilesetType)
PASS   : tst_App::openClose(TilesetType)
PASS   : tst_App::openClose(ImageType)
PASS   : tst_App::openClose(LayeredImageType)
PASS   : tst_App::saveTilesetProject()
PASS   : tst_App::saveAsAndLoadTilesetProject()
PASS   : tst_App::saveAsAndLoad(TilesetType)
PASS   : tst_App::saveAsAndLoad(LayeredImageType)
PASS   : tst_App::versionCheck(version-check-v0.2.1.slp)
PASS   : tst_App::loadTilesetProjectWithInvalidTileset()
PASS   : tst_App::loadLayeredImageProjectAfterTilesetProject()
PASS   : tst_App::animationPlayback()
PASS   : tst_App::keyboardShortcuts()
PASS   : tst_App::optionsShortcutCancelled()
PASS   : tst_App::optionsTransparencyCancelled()
PASS   : tst_App::showGrid()
PASS   : tst_App::undoPixels()
PASS   : tst_App::undoLargePixelPen()
PASS   : tst_App::undoTiles()
PASS   : tst_App::undoWithDuplicates()
PASS   : tst_App::undoTilesetCanvasSizeChange()
PASS   : tst_App::undoImageCanvasSizeChange()
FAIL!  : tst_App::undoImageSizeChange() Compared values are not the same
   Actual   (imageProject->image()->pixelColor(QPoint(0, 0))): #ffffffff
   Expected (imageCanvas->penForegroundColour())             : #ff000000
   Loc: [/Users/mitch/dev/slate/tests/auto/tst_app.cpp(1319)]
FAIL!  : tst_App::undoLayeredImageSizeChange() Compared values are not the same
   Actual   (layeredImageProject->currentLayer()->image()->pixelColor(0, 0)): #ffffffff
   Expected (QColor(Qt::black))                                             : #ff000000
   Loc: [/Users/mitch/dev/slate/tests/auto/tst_app.cpp(1366)]
PASS   : tst_App::undoPixelFill()
PASS   : tst_App::undoTileFill()
PASS   : tst_App::undoThickPen()
PASS   : tst_App::colours(TilesetType)
PASS   : tst_App::colours(ImageType)
PASS   : tst_App::colours(LayeredImageType)
PASS   : tst_App::colourPickerSaturationHex()
PASS   : tst_App::panes()
PASS   : tst_App::altEyedropper()
PASS   : tst_App::eyedropper()
PASS   : tst_App::eyedropperBackgroundColour()
PASS   : tst_App::zoomAndPan()
PASS   : tst_App::zoomAndCentre()
PASS   : tst_App::penWhilePannedAndZoomed(TilesetType, {40, 0}, 1)
PASS   : tst_App::penWhilePannedAndZoomed(TilesetType, {0, 40}, 2)
PASS   : tst_App::penWhilePannedAndZoomed(TilesetType, {40, 40}, 3)
PASS   : tst_App::penWhilePannedAndZoomed(TilesetType, {-40, 0}, 3)
PASS   : tst_App::penWhilePannedAndZoomed(TilesetType, {0, 0}, 2)
PASS   : tst_App::penWhilePannedAndZoomed(TilesetType, {-40, -40}, 2)
PASS   : tst_App::penWhilePannedAndZoomed(ImageType, {40, 0}, 1)
PASS   : tst_App::penWhilePannedAndZoomed(ImageType, {0, 40}, 2)
PASS   : tst_App::penWhilePannedAndZoomed(ImageType, {40, 40}, 3)
PASS   : tst_App::penWhilePannedAndZoomed(ImageType, {-40, 0}, 3)
PASS   : tst_App::penWhilePannedAndZoomed(ImageType, {0, 0}, 2)
PASS   : tst_App::penWhilePannedAndZoomed(ImageType, {-40, -40}, 2)
PASS   : tst_App::penWhilePannedAndZoomed(LayeredImageType, {40, 0}, 1)
PASS   : tst_App::penWhilePannedAndZoomed(LayeredImageType, {0, 40}, 2)
PASS   : tst_App::penWhilePannedAndZoomed(LayeredImageType, {40, 40}, 3)
PASS   : tst_App::penWhilePannedAndZoomed(LayeredImageType, {-40, 0}, 3)
PASS   : tst_App::penWhilePannedAndZoomed(LayeredImageType, {0, 0}, 2)
PASS   : tst_App::penWhilePannedAndZoomed(LayeredImageType, {-40, -40}, 2)
PASS   : tst_App::useTilesetSwatch()
PASS   : tst_App::tilesetSwatchContextMenu()
PASS   : tst_App::tilesetSwatchNavigation()
PASS   : tst_App::cursorShapeAfterClickingLighter()
PASS   : tst_App::colourPickerHexField()
PASS   : tst_App::colourPickerHexFieldTranslucent()
PASS   : tst_App::eraseImageCanvas(ImageType)
PASS   : tst_App::eraseImageCanvas(LayeredImageType)
PASS   : tst_App::splitterSettingsMouse(TilesetType)
PASS   : tst_App::splitterSettingsMouse(ImageType)
PASS   : tst_App::splitterSettingsMouse(LayeredImageType)
PASS   : tst_App::autoSwatch(TilesetType)
PASS   : tst_App::autoSwatch(ImageType)
PASS   : tst_App::autoSwatch(LayeredImageType)
SKIP   : tst_App::autoSwatchGridViewContentY() Flaky experimental stuff
   Loc: [/Users/mitch/dev/slate/tests/auto/tst_app.cpp(2315)]
PASS   : tst_App::autoSwatchPasteConfirmation()
QWARN  : tst_App::swatches() DelegateModel::cancel: index out range 123 0
QWARN  : tst_App::swatches() qrc:/qml/ui/SwatchPanel.qml:224:5: QML RenameSwatchColourDialog: Binding loop detected for property "implicitWidth"
PASS   : tst_App::swatches()
PASS   : tst_App::selectionToolImageCanvas()
PASS   : tst_App::selectionToolTileCanvas()
PASS   : tst_App::cancelSelectionToolImageCanvas()
FAIL!  : tst_App::moveSelectionImageCanvas(ImageType, white background) Compared values are not the same
   Actual   (canvas->currentProjectImage()->pixelColor(4, 4)): #ffffffff
   Expected (QColor(Qt::black))                              : #ff000000
   Loc: [/Users/mitch/dev/slate/tests/auto/tst_app.cpp(2620)]
FAIL!  : tst_App::moveSelectionImageCanvas(ImageType, transparent background) Compared values are not the same
   Actual   (canvas->currentProjectImage()->pixelColor(4, 4)): #00000000
   Expected (QColor(Qt::black))                              : #ff000000
   Loc: [/Users/mitch/dev/slate/tests/auto/tst_app.cpp(2620)]
FAIL!  : tst_App::moveSelectionImageCanvas(LayeredImageType, white background) Compared values are not the same
   Actual   (canvas->currentProjectImage()->pixelColor(4, 4)): #ffffffff
   Expected (QColor(Qt::black))                              : #ff000000
   Loc: [/Users/mitch/dev/slate/tests/auto/tst_app.cpp(2620)]
FAIL!  : tst_App::moveSelectionImageCanvas(LayeredImageType, transparent background) Compared values are not the same
   Actual   (canvas->currentProjectImage()->pixelColor(4, 4)): #00000000
   Expected (QColor(Qt::black))                              : #ff000000
   Loc: [/Users/mitch/dev/slate/tests/auto/tst_app.cpp(2620)]
FAIL!  : tst_App::moveSelectionWithKeysImageCanvas() Compared values are not the same
   Actual   (imageProject->image()->pixelColor(4, 4)): #ffffffff
   Expected (QColor(Qt::black))                      : #ff000000
   Loc: [/Users/mitch/dev/slate/tests/auto/tst_app.cpp(2702)]
PASS   : tst_App::deleteSelectionImageCanvas(ImageType)
PASS   : tst_App::deleteSelectionImageCanvas(LayeredImageType)
FAIL!  : tst_App::copyPaste(ImageType) Compared values are not the same
   Actual   (canvas->currentProjectImage()->pixelColor(14, 14)): #ffffffff
   Expected (QColor(Qt::black))                                : #ff000000
   Loc: [/Users/mitch/dev/slate/tests/auto/tst_app.cpp(2790)]
FAIL!  : tst_App::copyPaste(LayeredImageType) Compared values are not the same
   Actual   (canvas->currentProjectImage()->pixelColor(14, 14)): #ffffffff
   Expected (QColor(Qt::black))                                : #ff000000
   Loc: [/Users/mitch/dev/slate/tests/auto/tst_app.cpp(2790)]
PASS   : tst_App::undoCopyPasteWithTransparency()
PASS   : tst_App::pasteFromExternalSource(ImageType)
PASS   : tst_App::pasteFromExternalSource(LayeredImageType)
PASS   : tst_App::flipPastedImage()
PASS   : tst_App::flipOnTransparentBackground()
PASS   : tst_App::selectionEdgePan(top-left)
PASS   : tst_App::selectionEdgePan(top)
WARNING: tst_App::selectionEdgePan(top-right) Mouse event at 1401, -1361 occurs outside of target window (1401x675).
WARNING: tst_App::selectionEdgePan(top-right) Mouse event at 1401, -1361 occurs outside of target window (1401x675).
PASS   : tst_App::selectionEdgePan(top-right)
WARNING: tst_App::selectionEdgePan(right) Mouse event at 1401, 40 occurs outside of target window (1401x675).
WARNING: tst_App::selectionEdgePan(right) Mouse event at 1401, 40 occurs outside of target window (1401x675).
PASS   : tst_App::selectionEdgePan(right)
WARNING: tst_App::selectionEdgePan(bottom-right) Mouse event at 1401, 1441 occurs outside of target window (1401x675).
WARNING: tst_App::selectionEdgePan(bottom-right) Mouse event at 1401, 1441 occurs outside of target window (1401x675).
PASS   : tst_App::selectionEdgePan(bottom-right)
WARNING: tst_App::selectionEdgePan(bottom) Mouse event at 256, 1441 occurs outside of target window (1401x675).
WARNING: tst_App::selectionEdgePan(bottom) Mouse event at 256, 1441 occurs outside of target window (1401x675).
PASS   : tst_App::selectionEdgePan(bottom)
WARNING: tst_App::selectionEdgePan(bottom-left) Mouse event at -1401, 1441 occurs outside of target window (1401x675).
WARNING: tst_App::selectionEdgePan(bottom-left) Mouse event at -1401, 1441 occurs outside of target window (1401x675).
PASS   : tst_App::selectionEdgePan(bottom-left)
PASS   : tst_App::selectionEdgePan(left)
PASS   : tst_App::panThenMoveSelection()
PASS   : tst_App::selectionCursorGuide()
PASS   : tst_App::rotateSelection(ImageType)
PASS   : tst_App::rotateSelection(LayeredImageType)
PASS   : tst_App::rotateSelectionAtEdge(ImageType)
PASS   : tst_App::rotateSelectionAtEdge(LayeredImageType)
PASS   : tst_App::rotateSelectionTransparentBackground(ImageType)
PASS   : tst_App::rotateSelectionTransparentBackground(LayeredImageType)
PASS   : tst_App::fillImageCanvas(ImageType)
PASS   : tst_App::fillImageCanvas(LayeredImageType)
PASS   : tst_App::fillLayeredImageCanvas()
PASS   : tst_App::greedyPixelFillImageCanvas(ImageType)
PASS   : tst_App::greedyPixelFillImageCanvas(LayeredImageType)
PASS   : tst_App::texturedFill(ImageType)
PASS   : tst_App::texturedFill(LayeredImageType)
PASS   : tst_App::pixelLineToolImageCanvas(ImageType)
PASS   : tst_App::pixelLineToolImageCanvas(LayeredImageType)
PASS   : tst_App::pixelLineToolTransparent(ImageType)
PASS   : tst_App::pixelLineToolTransparent(LayeredImageType)
PASS   : tst_App::rulersAndGuides(TilesetType)
PASS   : tst_App::rulersAndGuides(ImageType)
PASS   : tst_App::rulersAndGuides(LayeredImageType)
PASS   : tst_App::recentFiles()
PASS   : tst_App::addAndRemoveLayers()
PASS   : tst_App::layerVisibility()
PASS   : tst_App::moveLayerUpAndDown()
PASS   : tst_App::mergeLayerUpAndDown()
PASS   : tst_App::renameLayers()
PASS   : tst_App::duplicateLayers()
PASS   : tst_App::saveAndLoadLayeredImageProject()
PASS   : tst_App::layerVisibilityAfterMoving()
PASS   : tst_App::selectionConfirmedWhenSwitchingLayers()
PASS   : tst_App::newLayerAfterMovingSelection()
PASS   : tst_App::undoAfterMovingTwoSelections()
PASS   : tst_App::autoExport()
PASS   : tst_App::exportFileNamedLayers()
PASS   : tst_App::disableToolsWhenLayerHidden()
PASS   : tst_App::undoMoveContents()
PASS   : tst_App::undoMoveContentsOfVisibleLayers()
PASS   : tst_App::cleanupTestCase()
Totals: 131 passed, 9 failed, 1 skipped, 0 blacklisted, 31951ms
********* Finished testing of tst_App *********
16:45:56: /Users/mitch/dev/slate-qt5_slate_fw-Debug/qtc_qt5_slate_fw_Debug/test-app.99bdad93/test-app.app/Contents/MacOS/test-app exited with code 9
not-surt commented 5 years ago

Added button to select between square and circle brush, defaulting to square so it passes tests.

mitchcurtis commented 5 years ago

Nice! Thank you! I'll take a look soon.

mitchcurtis commented 5 years ago

I've taken a quick look and it seems to work well. I would like to add a test for the new features and make the icons a little less heavy, so would you mind retargeting this to a feature branch? Once I'm happy with it I'll merge it into master.

not-surt commented 5 years ago

Which base branch to you want me to use?

mitchcurtis commented 5 years ago

Just create a pen-tool branch perhaps?

not-surt commented 5 years ago

AFAIK I can't create branches on your repo.

mitchcurtis commented 5 years ago

Ah, didn't know that. :) How does https://github.com/mitchcurtis/slate/commits/pen-tool look? I just pushed to pen-tool from the checkout of your branch I had.

mitchcurtis commented 5 years ago

Arrrrgh I dunno what the hell I'm doing with this stuff. :D I tried pushing patches to the branch but somehow screwed it up...

mitchcurtis commented 5 years ago

Ah, you already changed the base branch - I didn't see that. :) Then I can just merge this pull request!

mitchcurtis commented 5 years ago

@not-surt, do you think there's a way to restore the earlier behaviour of determining where in a pixel the brush will draw? This is how it works now with the round brush:

pen-tool

Previously it would only go over to the next row/column if you go over the centre of the pixel, but now it goes into the top/left row/column if you go one tiny bit over the top/left, but won't go to the bottom/right until you go right past the bottom/right edge.

mitchcurtis commented 5 years ago

Here's another example but with the square brush:

pen-tool-2

mitchcurtis commented 5 years ago

Here's the original behaviour:

pen-tool-2

mitchcurtis commented 5 years ago

I wrote a test that can be used to verify the fix:

https://github.com/mitchcurtis/slate/commit/7700a1c4f0c41b3ca796cc434c85f34969649540

mitchcurtis commented 5 years ago

I think once that's fixed, this is good to be merged to master.

With regards to this:

Image canvas only so far.

I quickly checked because I couldn't remember, and the line tool doesn't work with tileset projects currently anyway, so that won't be an issue.

not-surt commented 5 years ago

Added a fix to snap brush correctly to pixels. Due to how QPainter handles 1 pixel wide lines they have to be snapped to pixel corner, rather than centre, in order to get clean lines, but results in them being asymmetrical.

not-surt commented 5 years ago

I quickly checked because I couldn't remember, and the line tool doesn't work with tileset projects currently anyway, so that won't be an issue.

I'm working on this. Having the canvas give the ApplyPixelLineCommand a list of image regions plus scene-space offset then redrawing the line offset in each region.

not-surt commented 5 years ago

Basic functionality in tile set projects is implemented here: https://github.com/not-surt/slate/tree/tiled_pen_tool

mitchcurtis commented 5 years ago

Oh wow, I only just noticed but you actually fixed #39 (presumably by using mLastPixelPenPressScenePositionF?) - nice!

Subpixel positioning looks good now. :)

Just one test failing, which I'll look into:

FAIL! : tst_App::useTilesetSwatch() 'imageGrabber.takeImage() != originalTilesetSnapshot' returned FALSE. () Loc: [/Users/mitch/dev/slate/tests/auto/tst_app.cpp(2172)]