Stamp Brush right-click cut when Shift is held

[kdx2a]

Implements #3961

Implementing this required a refactor of the state system in StampBrush. Behavior and State were previously handled in the same property, which resulted both in wrong behavior when using capture and pressing the behavior keys (replicate by holding right click, start holding shift, release right click, the behavior wasn't set to line as it should have). This commit refactors this system by splitting State and Behavior in two enums and two properties, resulting in making the implemented feature possible, fixing behavior bugs, and future-proofing this system for expansion.

[bjorn]

Thank you so much for the nice idea and for the contribution! It works great, though I have a few concerns:

Behavior and State were previously handled in the same property, which resulted both in wrong behavior when using capture and pressing the behavior keys (replicate by holding right click, start holding shift, release right click, the behavior wasn't set to line as it should have).

Indeed it looks like there is less code duplication with the enum split up. The issue was still there though. Due to setStamp(TileStamp()) in beginCapture and the condition in modifiersChanged that the stamp isn't empty, it wasn't updating the brush behavior. I've just removed the condition, since it didn't look like it was actually necessary.

I don't really like the code duplication of the erasing code in CaptureStampHelper::endCapture and Eraser::doErase. I realize it looks rather different but since it should be doing mostly the same thing, I think it could live in a shared place (like adding a MapDocument::eraseRegion). I think the Eraser::doErase implementation is preferred because it uses only a single undo command. I can look into changing this tomorrow, if you don't beat me to it.

Also, I wonder if we really need the "cut" state to be set when you start capturing? The code would be rather simpler if we'd just take Shift state at the end of the capture.

Finally, there is a CaptureStampHelper because there are a bunch of other tools that allow capturing the brush. Now, the Shift modifier only works for the Stamp Brush. Did you consider whether it can behave consistently for the tools derived from AbstractTileFillTool as well?

[eishiya]

Also, I wonder if we really need the "cut" state to be set when you start capturing? The code would be rather simpler if we'd just take Shift state at the end of the capture.

I think this would also be more intuitive for the user.

[kdx2a]

Thank you for making Tiled!

I don't really like the code duplication of the erasing code in CaptureStampHelper::endCapture and Eraser::doErase. I realize it looks rather different but since it should be doing mostly the same thing, I think it could live in a shared place (like adding a MapDocument::eraseRegion). I think the Eraser::doErase implementation is preferred because it uses only a single undo command. I can look into changing this tomorrow, if you don't beat me to it.

To be completely honest I'm not sure how to make this change properly, I'd need to get more familiar with this part of the codebase. I'll take a glance at it but I'll probably focus on the other bulletpoints tonight.

Also, I wonder if we really need the "cut" state to be set when you start capturing? The code would be rather simpler if we'd just take Shift state at the end of the capture.

I don't think it's necessary either. I'll make the change, if you decide you want to keep the state afterwise I'll let you reverse the commit.

Finally, there is a CaptureStampHelper because there are a bunch of other tools that allow capturing the brush. Now, the Shift modifier only works for the Stamp Brush. Did you consider whether it can behave consistently for the tools derived from AbstractTileFillTool as well?

I didn't consider it, no. My bad. I'll take a look and try to make it behave consistently.

[kdx2a]

Everything seems to work properly, although I noticed than cutting no tile an empty area (or deleting it with the Eraser tool for all that matter) now creates a Cut history entry that does nothing. I'm not sure if this is intended behavior or not?

[bjorn]

I'm not sure if this is intended behavior or not?

As discussed on Discord, it might be better to avoid the needless history entry, but this is the existing behavior so we'll merge this PR as-is.

Thanks again!