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 flip X and Flip Y, and Rotation (90, 180, 270) buttons to BaseTool #988

Merged
merged 31 commits into from
Mar 23, 2024

Conversation

RorotoSic
Copy link
Contributor

This will be really useful for custom brushes. (rather than creating 8 different brushes, it will be just one) ^^

image

@OverloadedOrama
Copy link
Member

This is a very nice addition and it seems very helpful! But I am not sure if this is the best way to implement it UI-wise.

I am not sure if these options should be there. It might be a good idea to add a new "Brush settings" popup where these settings can be placed. This means that users will have to do more clicks in order to make these changes, but the UI will be more clean, tidy and simple. I fear that the current way may make the UI too bloated.

For example, Krita has an "Edit brushes" button at the top of the UI. In Pixelorama, it could be placed in the Global Tool Options (next to mirror, pixel perfect and dynamics buttons). This could also help us in the long run if we decide to make the brush system even more powerful and configurable in the future.

Even if we decide that they should remain as they are, I have some notes:

  • The new buttons are always visible, even when a non-image brush is selected. This shouldn't happen, as these buttons have no effect on non-image brushes and they take a lot of space for no reason. They should be hidden when not used, similarly to the "Brush color from" slider.
  • These settings should be below the sliders, along with the rest of the CheckBoxes.
  • Perhaps some buttons can be removed. Flip Y is not needed, as enabling Flip X and R 180 achieves the same result.
  • I am not sure if we should allow multiple "R" buttons to be selected at the same time. Perhaps they can be radio buttons instead (CheckBoxes with the same ButtonGroup that allows them to be unselected)

@RorotoSic
Copy link
Contributor Author

RorotoSic commented Mar 7, 2024

I think it's always best to reduce the number of clicks as much as possible, especially in art software, to keep the user in the creative flow.
I also thought about putting buttons on the tool's global options, but they will aplly modifications on all tools like "pixel perfect" do and its better to have each tools their own modificiations.
(Personaly in krita i only use the "Edit brushes" to CREAT new brush , never to update one during painting process, but it can be just me.)

  • True and correction made
  • It's quicker to update brush when flip/rotate are above. and i plan to code an option to keep brush folder open to preview flip and rotate modifcation in live to unlock creative posibility by seeing them that we may not thinked about. Can i sugest to add an expand button to keep the ui clean (as i made in the gif) ?
  • It's true, same result but more clicks and a little brain work ^^ it's always easier to do with a dedicated button
  • I modified the radio buttons and I don't know if I like it. Previously the rotations were applied on the brush we see in the preview (that brush may already had some flip and rotate changes), now with the radio button our brain has to remember the original brush (the one without changes ), remove the modification activated that we see on preview and then apply the desired change. Both approaches works, i dont know wich one is better.

ezgif-1-659ce0a639

i dont add txt to the Translations.pot bc not sure to understand and dont want to make mistake
@OverloadedOrama
Copy link
Member

I think the expand button works fine. We may need to reconsider the UI in the future if we add more options, but for now I think it should be fine. By the way, we have a custom CollapsibleContainer node (which can be added by adding a new node in the Scene tree and searching "CollapsibleContainer"), and you can put the elements that can be collapsed/expanded as children of that node, without needing to add any extra code for hiding/unhiding. You can also add text to the CollapsibleContainer node, perhaps something like "Rotation options".

As for the radio button, I don't really have a strong opinion. I feel like it makes more sense this way, but I'm not totally against your original implementation, so feel free to choose which implementation you prefer.

@RorotoSic
Copy link
Contributor Author

RorotoSic commented Mar 16, 2024

I think as radio buttons seems to be more common we can keep them.
here is the current state (link for good video quality)
https://github.com/Orama-Interactive/Pixelorama/assets/70805756/f2767771-af66-4944-be64-8576ecfdb197

ezgif-2-c5b97f7762

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.

Okay, changes in the UI look good now I think! Left some nitpick comments about the code, and it should be good to get merged after that.

@@ -1,6 +1,7 @@
extends Node

signal color_changed(color, button)
signal flip_rotate(flip_x, flip_y, rotate_90, rotate_180, rotate_270)
Copy link
Member

Choose a reason for hiding this comment

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

[node name="RotationOptions" type="VBoxContainer" parent="." index="2"]
visible = false
layout_mode = 2
theme = SubResource("Theme_i7q3e")
Copy link
Member

Choose a reason for hiding this comment

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

I think adding an entire theme just to remove the CollapsibleContainer's internal button decoration might be a bit overkill. Buttons have a flat property that does exactly that. In the latest commit on the master branch, I added a flat property in CollapsibleContainer that updates the flat status of its internal button. I think this could be used instead of a theme.

signal color_changed(color: Color, button: int)
@RorotoSic
Copy link
Contributor Author

"signal flip_rotated(flip_x, flip_y, rotate_90, rotate_180, rotate_270) " was not t a conflict before , i dont know why sudenlly it is while nothing has changed in my code

@OverloadedOrama
Copy link
Member

Nothing to worry about, it's because I made a change to the master branch and it happened to affect that specific line. I fixed the conflict now.

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.

Should be good to get merged now. Excellent work, thank you very much!

@OverloadedOrama OverloadedOrama merged commit 8bfd474 into Orama-Interactive:master Mar 23, 2024
4 checks passed
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.

None yet

2 participants