Implement tool loop for selection tools by Liebranca · Pull Request #5222 · aseprite/aseprite

Liebranca · 2025-06-11T22:40:39Z

This adds a new tool loop for use with selection tools (and slice, too): it simply makes it so one can see their strokes no matter the zoom level. Fix #2852.

For testing purposes (comparing the old tool loop to the new one), I've added a bool to toggle between them: holding down the keys for a 'change brush size' action while in standby state with a selection tool will enable the old implementation, and 'deselect all' re-enables the new one.

I am unsure whether it would be best to use a solid color for drawing the mask or a marching ants pattern, and which color or colors to use for it. There are many possibilities with this, could be worth it to make it customizable later on. I was tempted to make the colors depend on whether one is adding, subtracting, replacing or intersecting, but I held off on it as it seemed like too much.

Anyway, for now I've made it so the new implementation looks almost identical to the original, so as to not overcomplicate things. I've left a few additional switches to quickly swap between some alternatives, again just for testing purposes:

With kReplaceSelection modifier (default), the mask is drawn over the ink as a solid black, so it almost looks the same as the old implementation.
With kAddSelection modifier, only the mask is used.
With kSubtractSelection modifier, the mask is drawn over the ink using a marching ants pattern.
With kIntersectSelection modifier, only the mask is used, and drawk with a marching ants pattern.

Side note: I think that for the defaut case the mask could be disabled when zoomed in, as it's not necessary, and activated only when zoomed out and the ink becomes less visible. I didn't add this just to keep things simple, but it's fairly easy to do.

Finally, there's only a few minor details left to handle:

When selecting, the mask can draw over the right and bottom edges of the canvas, and these are not always cleared, so the mask leaves a 'trail' over them. I couldn't find a way to redraw these two edges, save for forcing an updateEditor(), but I can tell that's not quite right. Needless to say, this is actually unnoticeable if the mask is the same color as the edges.
Because I needed to access the mask from both the tool loop instance and the editor itself, I wasn't entirely sure where it was best to store it. I've simply added it to the editor class for now, in the new m_selectionToolMask member variable, but maybe there's a more appropriate place for it.
In order to avoid code duplication, I made a second base class for tool loops, from which the old and new implementation are derived. This one essentially just holds all the methods and member variables that I am certain are needed by both, however, I haven't yet removed logic specific to slice/selection tools from the old implementation: this is so it can still be used for testing/comparison purposes.
Following from the last point, this second base class would have to be slightly reworked if the old implementation is to be stripped of logic specific to slice/selection tools. But this is mostly just moving code from the base class to the derived one.

aseprite-bot

clang-tidy made some suggestions

aseprite-bot · 2025-06-11T22:44:28Z

+  // Used by selection tools
+  virtual bool isSelectionToolLoop() const = 0;
+  virtual void addSelectionToolPoint(const gfx::Rect& rc) = 0;
+  virtual void clearSelectionToolMask(const bool finalStep) = 0;


warning: parameter 'finalStep' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

Suggested change

virtual void clearSelectionToolMask(const bool finalStep) = 0;

virtual void clearSelectionToolMask(bool finalStep) = 0;

aseprite-bot · 2025-06-11T22:44:28Z

  void drawSpriteUnclippedRect(ui::Graphics* g, const gfx::Rect& rc);
  void drawMaskSafe();
-  void drawMask(ui::Graphics* g);
+  enum MaskIndex { Document, SelectionTool, Count };


warning: enum 'MaskIndex' uses a larger base type ('unsigned int', size: 4 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]

enum MaskIndex { Document, SelectionTool, Count }; ^

aseprite-bot · 2025-06-11T22:44:28Z

+    if (isSelectionPreview &&
+        (!m_document->isMaskVisible() ||
+         (int(getModifiers()) & int(tools::ToolLoopModifiers::kReplaceSelection)) != 0)) {
+      Mask emptyMask;


warning: variable 'emptyMask' of type 'Mask' can be declared 'const' [misc-const-correctness]

Suggested change

Mask emptyMask;

Mask const emptyMask;

aseprite-bot · 2025-06-11T22:44:28Z

+      // Paint ink
+      if (m_ink->isPaint()) {
+        try {
+          ContextReader reader(m_context, 500);


warning: variable 'reader' of type 'ContextReader' can be declared 'const' [misc-const-correctness]

Suggested change

ContextReader reader(m_context, 500);

ContextReader const reader(m_context, 500);

aseprite-bot · 2025-06-11T22:44:28Z

+      if (m_ink->isPaint()) {
+        try {
+          ContextReader reader(m_context, 500);
+          ContextWriter writer(reader);


warning: variable 'writer' of type 'ContextWriter' can be declared 'const' [misc-const-correctness]

Suggested change

ContextWriter writer(reader);

ContextWriter const writer(reader);

aseprite-bot · 2025-06-11T22:44:28Z

+    // Start with an empty mask if the user is selecting with "default selection mode"
+    if (!m_document->isMaskVisible() ||
+        (int(getModifiers()) & int(tools::ToolLoopModifiers::kReplaceSelection)) != 0) {
+      Mask emptyMask;


warning: variable 'emptyMask' of type 'Mask' can be declared 'const' [misc-const-correctness]

Suggested change

Mask emptyMask;

Mask const emptyMask;

aseprite-bot · 2025-06-11T22:44:29Z

                                  const tools::Pointer::Button button,
                                  const bool convertLineToFreehand,
-                                  const bool selectTiles);
+                                  const bool selectTiles,


warning: parameter 'selectTiles' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

Suggested change

const bool selectTiles,

bool selectTiles,

aseprite-bot · 2025-06-11T22:44:29Z

                                  const bool convertLineToFreehand,
-                                  const bool selectTiles);
+                                  const bool selectTiles,
+                                  const bool selectionToolLoopEnabled);


warning: parameter 'selectionToolLoopEnabled' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

Suggested change

const bool selectionToolLoopEnabled);

bool selectionToolLoopEnabled);

dacap · 2025-06-13T13:49:20Z

This adds a new tool loop for use with selection tools (and slice, too): it simply makes it so one can see their strokes no matter the zoom level. Fix #2852.

Hi @Liebranca, good first draft! 👍

For testing purposes (comparing the old tool loop to the new one), I've added a bool to toggle between them: holding down the keys for a 'change brush size' action while in standby state with a selection tool will enable the old implementation, and 'deselect all' re-enables the new one.

As I said in the email, probably we can add a new experimental flag for this, and thinking a little more, probably we should make the option available in Edit > Preferences > Experimental, it's highly probable that some users might prefer to keep the current behavior anyway.

I am unsure whether it would be best to use a solid color for drawing the mask or a marching ants pattern, and which color or colors to use for it.

As a first implementation I'd use a static marching ant pattern with the same black & white (like the kIntersectSelection implementation but with black & white colors).

Anyway something important: We shouldn't use the ExpandCelCanvas in this new implementation, we should paint the feedback directly on the editor. ExpandCelCanvas creates a cel of the full sprite size and it's something we want to avoid for selection tools. Should improve the performance a lot when we select big images.

Side note: I think that for the defaut case the mask could be disabled when zoomed in, as it's not necessary, and activated only when zoomed out and the ink becomes less visible. I didn't add this just to keep things simple, but it's fairly easy to do.

As we should remove the ExpandCelCanvas, the ink feedback will be removed and only the mask should be visible.

When selecting, the mask can draw over the right and bottom edges of the canvas, and these are not always cleared, so the mask leaves a 'trail' over them.

I didn't see this in detail, I think Editor::drawBackground() should do the job, although we can fix this later.

Because I needed to access the mask from both the tool loop instance and the editor itself, I wasn't entirely sure where it was best to store it. I've simply added it to the editor class for now, in the new m_selectionToolMask member variable, but maybe there's a more appropriate place for it.

We can have this variable, probably this could be a static member variable shared between all editors (and nullptr by default, only valid when a SelectionToolLoopImpl is alive).

In order to avoid code duplication, I made a second base class for tool loops, from which the old and new implementation are derived. This one essentially just holds all the methods and member variables that I am certain are needed by both, however, I haven't yet removed logic specific to slice/selection tools from the old implementation: this is so it can still be used for testing/comparison purposes.

I think m_expandCelCanvas should be part of ToolLoopImpl only.

Following from the last point, this second base class would have to be slightly reworked if the old implementation is to be stripped of logic specific to slice/selection tools. But this is mostly just moving code from the base class to the derived one.

At the moment (and probably for several versions) we will keep the old implementation working (so the user can disable this new experimental implementation, that if it works ok, should be enabled by default in future versions).

aseprite-bot

clang-tidy made some suggestions

aseprite-bot · 2025-06-24T09:13:00Z


+void Editor::makeSelectionToolMask()
+{
+  m_selectionToolMask.reset(new Mask());


warning: method 'makeSelectionToolMask' can be made static [readability-convert-member-functions-to-static]

src/app/ui/editor/editor.h:320:

- void makeSelectionToolMask(); + static void makeSelectionToolMask();

aseprite-bot · 2025-06-24T09:13:00Z

+
+void Editor::deleteSelectionToolMask()
+{
+  m_selectionToolMask.reset();


warning: method 'deleteSelectionToolMask' can be made static [readability-convert-member-functions-to-static]

src/app/ui/editor/editor.h:321:

- void deleteSelectionToolMask(); + static void deleteSelectionToolMask();

aseprite-bot · 2025-06-24T09:13:01Z

+
+bool Editor::hasSelectionToolMask()
+{
+  return m_selectionToolMask && !m_selectionToolMask->isEmpty();


warning: method 'hasSelectionToolMask' can be made static [readability-convert-member-functions-to-static]

src/app/ui/editor/editor.h:322:

- bool hasSelectionToolMask(); + static bool hasSelectionToolMask();

aseprite-bot · 2025-06-24T09:13:01Z

  // an Editor or EditorState event.
  void showUnhandledException(const std::exception& ex, const ui::Message* msg);

+  Mask* getSelectionToolMask() { return m_selectionToolMask.get(); }


warning: method 'getSelectionToolMask' can be made static [readability-convert-member-functions-to-static]

Suggested change

Mask* getSelectionToolMask() { return m_selectionToolMask.get(); }

static Mask* getSelectionToolMask() { return m_selectionToolMask.get(); }

aseprite-bot · 2025-06-24T09:13:01Z

+      return false;
    }
+    else
+      return ToolLoopBase::needsCelCoordinates();


warning: do not use 'else' after 'return' [readability-else-after-return]

Suggested change

return ToolLoopBase::needsCelCoordinates();

return ToolLoopBase::needsCelCoordinates();

aseprite-bot · 2025-06-24T09:13:01Z

+        Slice* slice = new Slice;
+        slice->setName(getUniqueSliceName());
+
+        SliceKey key(bounds);


warning: variable 'key' of type 'SliceKey' can be declared 'const' [misc-const-correctness]

Suggested change

SliceKey key(bounds);

SliceKey const key(bounds);

aseprite-bot · 2025-06-24T09:13:01Z

+
+private:
+  // EditorObserver impl
+  void onScrollChanged(Editor* editor) override { updateAllVisibleRegion(); }


warning: parameter 'editor' is unused [misc-unused-parameters]

Suggested change

void onScrollChanged(Editor* editor) override { updateAllVisibleRegion(); }

void onScrollChanged(Editor* /*editor*/) override { updateAllVisibleRegion(); }

aseprite-bot · 2025-06-24T09:13:01Z

+private:
+  // EditorObserver impl
+  void onScrollChanged(Editor* editor) override { updateAllVisibleRegion(); }
+  void onZoomChanged(Editor* editor) override { updateAllVisibleRegion(); }


warning: parameter 'editor' is unused [misc-unused-parameters]

Suggested change

void onZoomChanged(Editor* editor) override { updateAllVisibleRegion(); }

void onZoomChanged(Editor* /*editor*/) override { updateAllVisibleRegion(); }

aseprite-bot · 2025-06-24T09:13:01Z

+  void onScrollChanged(Editor* editor) override { updateAllVisibleRegion(); }
+  void onZoomChanged(Editor* editor) override { updateAllVisibleRegion(); }
+
+  std::string getUniqueSliceName() const


warning: method 'getUniqueSliceName' can be made static [readability-convert-member-functions-to-static]

Suggested change

std::string getUniqueSliceName() const

static std::string getUniqueSliceName()

aseprite-bot · 2025-06-24T09:13:02Z

+  std::string getUniqueSliceName() const
+  {
+    std::string prefix = "Slice";
+    int max = 0;


warning: variable 'max' of type 'int' can be declared 'const' [misc-const-correctness]

Suggested change

int max = 0;

int const max = 0;

dacap · 2025-08-07T22:54:16Z

I think it's a pretty good first implementation to go forward. I'll merge it as it is in the beta branch and do some minor changes to the code after that.

* Added a new Editor::drawMaskBoundaries() and Editor::m_selectionToolMaskBoundaries to store the generated boundaries for the active SelectionToolLoopImpl. * Removed app::Doc::setMaskBoundaries() used to restore the active mask boundaries. * Removed Editor::MaskIndex

Implement tool loop for selection tools

b1d42a2

Liebranca changed the base branch from main to beta June 11, 2025 22:42

aseprite-bot reviewed Jun 11, 2025

View reviewed changes

Remove m_expandCelCanvas from SelectionToolLoopImpl

b2afbf9

aseprite-bot reviewed Jun 24, 2025

View reviewed changes

dacap self-assigned this Jun 24, 2025

dacap marked this pull request as ready for review August 7, 2025 22:54

dacap self-requested a review as a code owner August 7, 2025 22:54

dacap merged commit 2dbfaf7 into aseprite:beta Aug 7, 2025
12 checks passed

This was referenced Feb 28, 2025

Improve selection tools feedback #2852

Closed

Rectangular Marquee Tool isn't visible when image is scaled < 100% #4318

Open

	virtual void clearSelectionToolMask(const bool finalStep) = 0;
	virtual void clearSelectionToolMask(bool finalStep) = 0;

	ContextReader reader(m_context, 500);
	ContextReader const reader(m_context, 500);

	ContextWriter writer(reader);
	ContextWriter const writer(reader);

	const bool selectionToolLoopEnabled);
	bool selectionToolLoopEnabled);

	Mask* getSelectionToolMask() { return m_selectionToolMask.get(); }
	static Mask* getSelectionToolMask() { return m_selectionToolMask.get(); }

	return ToolLoopBase::needsCelCoordinates();
	return ToolLoopBase::needsCelCoordinates();

	void onScrollChanged(Editor* editor) override { updateAllVisibleRegion(); }
	void onScrollChanged(Editor* /editor/) override { updateAllVisibleRegion(); }

	void onZoomChanged(Editor* editor) override { updateAllVisibleRegion(); }
	void onZoomChanged(Editor* /editor/) override { updateAllVisibleRegion(); }

	std::string getUniqueSliceName() const
	static std::string getUniqueSliceName()

Uh oh!

Uh oh!

Conversation

Liebranca commented Jun 11, 2025

Uh oh!

aseprite-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dacap commented Jun 13, 2025

Uh oh!

aseprite-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dacap commented Aug 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants