Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix stroke not being shown on a selection while drawing #1807

Merged
merged 3 commits into from
Jan 19, 2024

Conversation

MrStevns
Copy link
Member

How to reproduce:

  1. Create a selection
  2. Draw a stroke inside it

Expected:
Stroke is drawn

Actual:
Stroke is not drawn until you stop drawing.

@J5lx J5lx added this to the v0.6.7 milestone Jan 17, 2024
@J5lx J5lx self-assigned this Jan 17, 2024
Copy link
Member

@J5lx J5lx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this. I noticed this fix assumes that isDrawing implies mSelectionTransform.isIdentity(), which isn’t necessarily true, and thereby uncovers another issue:

Steps to reproduce:

  • Switch to selection tool and make a selection
  • Hold alt and drag any of the corner handles to transform the selection
  • Switch to, say, pencil tool and draw across the selection.

Now while drawing across the selection, both the stroke and the selection will be drawn as if the selection was not transformed, but once you stop drawing both will be transformed.

I still have a bunch of other things to worry about unfortunately, so I’ll let you decide whether you want to treat it as separate issues and merge this as is or fix both in one go. It’s also why I still haven’t been able too lok at your other PR…

@MrStevns
Copy link
Member Author

MrStevns commented Jan 18, 2024

Hi Jacob, I hope you're doing well. I know that life can get in the way, so don't feel bad about not having reviewed the other PR yet, we'll get there eventually. 😃

You're right about that other bug! I completely forgot about the temporary move state 😅 ... The transformation should actually be in a identity state in cases where you are able to draw on the canvas again, because the "leavingThisTool" function should commit the transformation before going back to another tool.

I've pushed a fix for that too now and tested the cases that I could think of.

Copy link
Member

@J5lx J5lx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

The transformation should actually be in a identity state in cases where you are able to draw on the canvas again

I’ve added an assertion for this assumption to make it more explicit.

@J5lx J5lx merged commit 4eb1643 into pencil2d:master Jan 19, 2024
7 of 8 checks passed
@scribblemaniac
Copy link
Member

@MrStevns @J5lx I've had a chance to look at this. The tool change seem sensible. However, this isn't enough to solve the issue completely. If you create a selection, switch to a drawing tool, and move the selection with the arrow shortcuts, and then draw a stroke, then transform is not identity and isDrawing is true.

@MrStevns
Copy link
Member Author

MrStevns commented Jan 21, 2024

@scribblemaniac

I am not able to reproduce that behavior, are you referring to the 'M' shortcut? Once you switch from the Move tool back to a drawing tool, using a shortcut or not, that should still trigger the LeavingTool function and apply the transformation.
Or is there another shortcut that I've forgotten about?

edit: .... ah you meant the arrow keys on the keyboard.. 😅 bah yeah i completely forgot about those...
I will look into it.

selection-bug-investigation-trimmed.mp4

@MrStevns MrStevns deleted the fix-drawing-on-selection-not-shown branch February 5, 2024 19:12
@MrStevns
Copy link
Member Author

MrStevns commented Feb 5, 2024

If you create a selection, switch to a drawing tool, and move the selection with the arrow shortcuts, and then draw a stroke, then transform is not identity and isDrawing is true.

I've investigated this and can now say that it's a bug. Moving the selection when the current tool is Select, should not modify the keyframe, and therefore neither the selection transform.

The behavior should work as such:
Select tool: Using arrow keys while something is selected will only translate the selection rectangle
Move tool: Using arrow keys while something is selected will transform the bitmap/vector image.

I'll address this in my upcoming PR for floating selection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants