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

Add Rectangle and Ellipse tools #456

Merged
merged 17 commits into from
Mar 30, 2021

Conversation

Dwahgon
Copy link
Contributor

@Dwahgon Dwahgon commented Feb 15, 2021

Closes #359

This PR adds a tool that allows you to draw shapes, like rectangles and ellipses. It also allows you to draw shapes with a 1:1 aspect ratio (so you can draw circles and squares), by holding down the shift key.

TODO:

  • Cleaning up
  • Add the ability to move the shape's center to the click origin by pressing CTRL
  • Add blue theme buttons
  • Fix preview not working when drawing with the right mouse button
  • Rework brush size
  • Add config functions
  • Fix some pixels not being filled when drawing a tall 4 pixels wide ellipse
  • There's a bug where sometimes drawing a shape will spam the output with errors, but I haven't found a way to consistently reproduce it. I think this was fixed

I'll add images and gifs of the tool in action later today.

Edit 1: Added the ability to move the shape's center to the click origin, but with ALT instead of CTRL. I can change it if CTRL is preferable. Maybe we should add these commands to the keybindings?
I also added a checkbox that allows you to draw filled shapes. A better algorithm for drawing the filled ellipse might be needed.

Edit 2: I'm going to make this PR ready for review now. Here's a video showing off the tool in action:

Screencast.2021-02-16.15.09.25.mp4

@Dwahgon Dwahgon marked this pull request as ready for review February 16, 2021 18:13
@Dwahgon Dwahgon changed the title Add Shapes Tool (WIP) Add Shapes Tool Feb 16, 2021
@Variable-ind
Copy link
Contributor

Ok so i checked and it works great but i noticed two things:
firstly the preview rectangle does not appear if the tool is selected with right mouse button.
and secondly, the preview rectangle does not update as the size of the shape brush is changed you might want to change that

@Dwahgon
Copy link
Contributor Author

Dwahgon commented Feb 18, 2021

Ok so i checked and it works great but i noticed two things:
firstly the preview rectangle does not appear if the tool is selected with right mouse button.
and secondly, the preview rectangle does not update as the size of the shape brush is changed you might want to change that

I have addressed these two points in the latest commits.
During my tests with the shape thickness slider, I've noticed that sometimes drawing the shape will spam the console with errors, and I haven't found a consistent way of replicating it.
But other than that, the tool is mostly done, unless there is something I need to implement or I have forgotten to do something

@Variable-ind
Copy link
Contributor

Variable-ind commented Feb 18, 2021

OK, so i checked and it appears to work fine but an idea hit me,
what if we draw the preview rectangle using "Shift + click" and then paint using a simple click. i mean the idea is to be able to make the preview rectangle follow the mouse (similar to the preview of the pencil tool) and paint the preview shape whenever we click (if the rectangle belongs to the right-tool then right-click and vice versa)

@Dwahgon
Copy link
Contributor Author

Dwahgon commented Feb 18, 2021

OK, so i checked and it appears to work fine but an idea hit me,
what if we draw the preview rectangle using "Shift + click" and then paint using a simple click. i mean the idea is to be able to make the preview rectangle follow the mouse (similar to the preview of the pencil tool) and paint the preview shape whenever we click (if the rectangle belongs to the right-tool then right-click and vice versa)

I don't know if I understood correctly what you were asking for, but check out the commit I've just pushed. Press CTRL before releasing the mouse button to keep the preview on screen. Then, press again to confirm the preview. If you hold CTRL and press the mouse button instead, you will draw another preview, though it will not stay on screen if you release CTRL.

@Variable-ind
Copy link
Contributor

Variable-ind commented Feb 19, 2021

you under stood correctly. now the only thing left is to make the preview rectangles (or preview ellipse) follow the mouse till a mouse click (left for left tool and right for right tool) is pressed that would allow us to place the shape more accurately on the canvas

Exactly like what it does with the pencil tool here (except that for the shapes tool we can draw the shapes ourselves instead of just using a simple circle)

pixelorama-2021-02-19_14.38.17.mp4

@Dwahgon
Copy link
Contributor Author

Dwahgon commented Feb 19, 2021

you under stood correctly. now the only thing left is to make the preview rectangles (or preview ellipse) follow the mouse till a mouse click (left for left tool and right for right tool) is pressed that would allow us to place the shape more accurately on the canvas

Exactly like what it does with the pencil tool here (except that for the shapes tool we can draw the shapes ourselves instead of just using a simple circle)

pixelorama-2021-02-19_14.38.17.mp4

Ok, the shapes should follow the mouse now.

@Variable-ind
Copy link
Contributor

Yep, works like a charm

@Erevoid
Copy link
Member

Erevoid commented Feb 23, 2021

Extremely useful addition. I have some suggestions to make before it gets merged.

  1. While holding shift the preview is a bit buggy, it keeps changing from where the cursor is and where the shape should be
  2. The control button should be linked to the expand from center function, because alt+shift switches the language on windows machines, meaning it's not ideal for this purpose
  3. It would be preferable if the whole idea with the current control button and the way the preview works is removed. We are considering of potentially making something similar with the shapes being selected after drawn with the 0.9 update and the selection reworks.
  4. It would be nice if the tool options contained the word "size" to the left of the shape's size number and if it indicated the word "px" within the Spinbox, exactly like in the pencil options.
  5. It would be even better if the ellipse and rectangle shapes are two different tools, instead of one tool containing both.

Consider changing those elements and the PR is good to go!

@Dwahgon
Copy link
Contributor Author

Dwahgon commented Feb 23, 2021

Extremely useful addition. I have some suggestions to make before it gets merged.

  1. While holding shift the preview is a bit buggy, it keeps changing from where the cursor is and where the shape should be
  2. The control button should be linked to the expand from center function, because alt+shift switches the language on windows machines, meaning it's not ideal for this purpose
  3. It would be preferable if the whole idea with the current control button and the way the preview works is removed. We are considering of potentially making something similar with the shapes being selected after drawn with the 0.9 update and the selection reworks.
  4. It would be nice if the tool options contained the word "size" to the left of the shape's size number and if it indicated the word "px" within the Spinbox, exactly like in the pencil options.
  5. It would be even better if the ellipse and rectangle shapes are two different tools, instead of one tool containing both.

Consider changing those elements and the PR is good to go!

About point 1, could you attach a video or image showing this problem? For me, it seems to be working correctly, the preview matches the drawn shape, and the way the shape scales while holding shift and moving the mouse seems to be very similar to what you see in other pixel art software, like Piskel.
@Variable-ind asked for the implementation of the control button, so some discussion is needed to decide if we should remove it. I'll keep it for now, but I will change it to be the shift key instead.
The reason I added both shapes into one tool is to allow for the possibility of adding a variety of other shapes in the future, without polluting the toolbar.

@Variable-ind
Copy link
Contributor

Variable-ind commented Feb 23, 2021

How about this, we could remove the control. Instead we could set it in such a way that a single click will move the preview where the mouse is currently at and double-click will draw the shape (or an "Apply" button can simply be placed in the tool settings instead of double-click)

@OverloadedOrama
Copy link
Member

About point 1, could you attach a video or image showing this problem? For me, it seems to be working correctly, the preview matches the drawn shape, and the way the shape scales while holding shift and moving the mouse seems to be very similar to what you see in other pixel art software, like Piskel.

I can replicate the issue on Windows 10.

2021-02-23.21-31-42.mp4

In the video I am always holding the Shift button, and the end result is correct, but the preview is flickering.

@OverloadedOrama
Copy link
Member

As for the other points, I'd say to remove the Control functionality completely (and use Control for the behavior that is used in Alt) for now and discuss it again in the future once the new selection system has been finalized, in case we can use it to move the shape after it has been created. So, Shift for keeping aspect ratio, Control for expanding from center.

I also think putting the 2 shapes as 2 different tools would be better, it would allow easier switching for the users as they can map them to different keyboard shortcuts. New tools will be added in v0.9 so the pollution problem will most likely occur anyway, but that's a different issue. We could group tools together in a single icon, like GIMP and Aseprite does it, if it becomes a problem.

@Dwahgon
Copy link
Contributor Author

Dwahgon commented Feb 24, 2021

Alright, I think I've addressed most of these issues. I've made various changes so more testing is needed.

  1. While holding shift the preview is a bit buggy, it keeps changing from where the cursor is and where the shape should be

This is supposed to be fixed now

  1. The control button should be linked to the expand from center function, because alt+shift switches the language on windows machines, meaning it's not ideal for this purpose
  2. It would be preferable if the whole idea with the current control button and the way the preview works is removed. We are considering of potentially making something similar with the shapes being selected after drawn with the 0.9 update and the selection reworks.

The previous implementation of the ctrl button has been removed. Now, ctrl is used to draw 1:1 shapes.

  1. It would be nice if the tool options contained the word "size" to the left of the shape's size number and if it indicated the word "px" within the Spinbox, exactly like in the pencil options.

Added those as well

  1. It would be even better if the ellipse and rectangle shapes are two different tools, instead of one tool containing both.

Made them separate

@Dwahgon Dwahgon changed the title Add Shapes Tool Add Rectangle and Ellipse tools Feb 24, 2021
@Dwahgon
Copy link
Contributor Author

Dwahgon commented Mar 6, 2021

Should I change the expand from click origin button to be CTRL instead of ALT and make the draw 1:1 shape button be SHIFT, or the current configuration (CTRL = Draw 1:1 / ALT = Expand from click origin) is ok?

@Variable-ind
Copy link
Contributor

Variable-ind commented Mar 6, 2021 via email

@OverloadedOrama
Copy link
Member

Yeah I think Ctrl for expand from center and Shift for 1:1 might be better. In the future we'll make these shortcuts customizable for the users anyway, so everyone can have their own configuration. (#309)

@Dwahgon
Copy link
Contributor Author

Dwahgon commented Mar 6, 2021

Yeah I think Ctrl for expand from center and Shift for 1:1 might be better. In the future we'll make these shortcuts customizable for the users anyway, so everyone can have their own configuration. (#309)

Done. If I haven't missed anything, it should be good to go.

@Erevoid
Copy link
Member

Erevoid commented Mar 8, 2021

Looking nice, I too think it's good to go

Copy link
Member

@OverloadedOrama OverloadedOrama left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@OverloadedOrama OverloadedOrama merged commit 943a69c into Orama-Interactive:master Mar 30, 2021
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.

Circle and Rectangle shape tools
4 participants