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

The stack tool can delete stacks now. #4

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Teh-Mad-Cow
Copy link

The stack tool now switches to a toolset like the config tool, so that the secondary button deletes whole stacks. It supports brushes, lines, and boxes.
While it performs better than the stack tool when dragging, it could still use some improvement.

@krawthekrow
Copy link
Owner

  1. Stack tool already has a framework to gather all the particles affected by a draw. It would be better to reuse those instead of reimplementing draw functions.
  2. If floodfill isn't ready, it would make sense to omit DrawFill altogether for now.
  3. If stack tool is toggleable, "[Stk Tool]" or something like that should show at the top left of the debug HUD (see close to the end of OnDraw()).
  4. Having the delete-stack tool be connected to stack tool feels suboptimal from a user interface standpoint, since they perform fundamentally different functions. It might be more natural to make it part of stack mode. Here are two possible proposals:

(A) Make the default right-click action delete stacks, and make the stack mode right-click action delete single particles from stacks. In this proposal, unmodified-right-click in stack mode could also be made to delete only one particle per mouse-down. This would be most consistent with stack mode's existing intuition, but would make default right click laggy and mess up existing users' workflow.
(B) Keep the default right-click action the same, but have stack mode's right click delete entire stacks when ctrl or shift is held down, and delete only one particle per mouse-down otherwise. This would cast stack mode as a more general stack utility mode.

I'm leaning more towards (B), but let me know what you think.

@Teh-Mad-Cow
Copy link
Author

Teh-Mad-Cow commented Mar 5, 2022

  1. Stack tool already has a framework to gather all the particles affected by a draw. It would be better to reuse those instead of reimplementing draw functions.

I wasn't sure how to do that originally, but I think I got it. The exact implementation might vary, but is this reasonable? I could change how StackTool::ProcessParts uses parts based on whether a new flag is set in the sim: StackDeleteMode. Then when we're in [Stack] mode and detect the active tool is DEFAULT_PT_NONE (or something), we set the StackDeleteMode flag to true and override the active tool to StackTool. That way we can use that code to do the erasure. I'm open to any suggestions/corrections though.

  1. If floodfill isn't ready, it would make sense to omit DrawFill altogether for now.

Oops yeah I wasn't planning to continue work on it, till you mentioned how [Stack] mode treats clicks. I'm going to save that for last though, as it's still incredibly niche. I only wanna include it for completeness

As for points 3 and 4, yeah I'll go ahead and do that. Option B is the least disruptive to regular workflow. Took me a bit to come around, but yeah it really does fit there better in the [Stack] mode.

@krawthekrow
Copy link
Owner

Your changes had inspired me to rework the toolset framework a bit, so hold on the changes related to making the stack tool toggleable for now. I think I might end up just going ahead and merging that manually.

For (1), I would recommend abstracting those functions so that they return a vector of particles, and putting them in a separate file. The reason why I mentioned them is because its implementation of DrawLine is faster, and it's a good starting point if you're planning to port DrawFill. If this sounds complicated, it's fine if you just copy-paste your existing routines; I can do the refactor myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants