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

Update swatches after Replace command #1265

Merged
merged 8 commits into from
Aug 18, 2022

Conversation

davidlamhauge
Copy link
Contributor

I am a little confused over this PR.
it closes #1055 , but it only contains one new line of code. I feel I've overlooked something.
I've tested it several times though, and it seems to work.

@scribblemaniac scribblemaniac added Bug Color Palette 🔹 Minor PR (only one reviewer required) labels Sep 17, 2019
Copy link
Member

@scribblemaniac scribblemaniac left a comment

Choose a reason for hiding this comment

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

I think this one line should be added to updateItemColor instead of here because you'll always want the object colors to match the palette colors. Unfortunately this is not sufficient to close #1055 as far as I can tell. @Jose-Moreno can probably weigh in on this more, but it seems that it does fix the replace command updating the color on both bitmap and vector layers, and it updates the other mode or other scaling. It does not update the name (and I don't think it should so that's fine).

However, in the last post in #1055 brings up an issue with the palette updating that has not been addressed here yet, and that is the real-time updating. When you have a vector layer active and you change the color in the color box, it will automatically update the active palette color. However, the color palette widget does not update to reflect this. Personally, I don't think changing the color should update the palette unless the replace command is used to prevent accidental changes, but I think this behavior is something we need to discuss.

@davidlamhauge
Copy link
Contributor Author

I've moved the function so the palette and object are updated almost simultaneously.
I've also added the real-time updating of the swatch, but I too think it should be an act of will, to change the palette. Real-time updating opens up for many accidental changes to the palette, but now we can try it, and discuss it further.

@Jose-Moreno
Copy link
Member

@scribblemaniac Hmm I think partly due to the discussions that took place in #732 and #755 the result was commit da990cc made by chchwy, but that behavior was not ideal, so later on @candyface implemented #961 which allowed for both bitmap and vector layers to alter colors in the palette separately.

So after that the real time update of swatches for vector layers was removed because of that, but I agree that it wasn't 100% clear if it was a by-product or an intentional removal 🤔

I personally would like that updating the swatch updates it in real time for vector only, however I'm also thinking that considering the replace command was implemented to deal with the color swatch state update, it would not be coherent if we revert back to the real-time update despite being a more intuitive way of previewing the changes when using a vector workflow.

If I were to be totally honest, for me the way it should work is where the Bitmap palette behavior is always static and vector is always dynamic, so there's contrast between the two workflow "modes"

How swatches behave In relation to applied colors on canvas

  • Bitmap Swatches are Static (Changing a color swatch does not update the canvas)
  • Vector Swatches are Dynamic (Changing a color swatch does update the canvas)

How swatches behave In relation to the Color Inspector / wheel:

  • Bitmap Swatches are Static
  • Vector Swatches are Dynamic (Read proposal below)*

Proposal *: For vector swatches, they could change real-time through the color inspector but only when specifically modifying the selected active swatch through the select color dialog.

While it would be similar to the previous behavior, in order to give coherence to the replace command and avoiding changing the swatch inadvertently, -which was the main problem for users before it was excised mainly because people are accustomed to select the color first and then change it but didn't pay attention to the layer they were on- we could make this real-time update to happen only in this sort of "edit mode" for swatches that is the Select Color dialog.

In Harmony for example you can only change the color when you double click the swatch to "edit" and it will bring a panel similar to the color selector we already have in Pencil2D.

So in Pencil2D If you select the color using the Inspector / Wheel first, you could right click, press replace swatch and all is good.

But if you select the color swatch and then change the color in the inspector, then double click the swatch (or use a new command called Edit), the current color RGB values would be returned to the selector dialog to avoid retyping / re-selecting the color (this would also work when you select the swatch once, then double click so the starter color would be that of the swatch)

Then using the Select Color dialog slider, the rgb input boxes or if you picked a pre-defined color this would update the color in real time on canvas.

Lastly if you press cancel it would default back to the previous color the swatch had recorded (not the one on the inspector necessarily).

@davidlamhauge
Copy link
Contributor Author

I think I understand what you're writing @Jose-Moreno, but it's not easy for me. As said before, I've never worked with ToonBoom or other software, and I try to make sense in behaviour that I don't understand.
After reading your comment, I still think, that the behaviour that most users would find logical or easy to understand, is that the Bitmap layer is static as it is, but the Vector layer is dynamic in the sense that it updates the scribblearea in real-time, when you pick a color in the Color Wheel or Inspector. When you've found your preferred color, you right-click on the color and press 'Replace'.
It's not 100% real-time, but I think it would be easier to understand for the users.
That said - I'll implement whatever you guys decide. I have no plans on using the Vector layers myself.

@davidlamhauge
Copy link
Contributor Author

So - what can we agree upon in this issue? How and when should vector swatches be updated?

@MrStevns
Copy link
Member

MrStevns commented Sep 25, 2021

This has been sitting for too long, so I'll join in.
Here are my thoughts, after having a discussion with Jose on Discord.

I believe the functionality we want is:
* A vector stroke should not change unless it's been selected or the swatch "replace" command is used.

  • The swatch should not change unless the "replace" command is used

Any thoughts?

@MrStevns
Copy link
Member

MrStevns commented Sep 26, 2021

@pencil2d/developers
After taking a deeper look into this, it's probably out of scope to change the stroke only if selected, because the colors in the palette and the vector strokes are so attached to each other and there's no easy way to change that currently as far as I can see.

My proposal:
We remove the the ability to change a a vector stroke from the color wheel and color inspector. This means that only the front color will be updated when interacting with the inspector or color wheel, which will affect the stroke color on the bitmap layer but not on the vector layer.

The swatch will not change unless you use the replace command, which will then affect vector strokes.

With this change, you won't be able to see immediate color changes to a stroke on the vector layer anymore, you'd have to use the replace command specifically for that to happen.

Here's how it would work:
vector strokes

is that what we want?

…vector strokes

- Ensures that only by explicitly using the 'replace' command, attached vector strokes will change
- Fixes a bug that would detach a vector stroke from the palette
@Jose-Moreno
Copy link
Member

is that what we want?

@MrStevns Sorry!!! I honestly never saw this before until @J5lx pointed out today 🤦‍♂️ AFAIK part of this has already been implemented (the not updating the vector strokes) The behavior you presented is exactly what we ended up discussing after some time, so with the proper updating to the current master I think it can be merged in the way you explained.

@MrStevns
Copy link
Member

The proposed changes has been added to the PR, it's ready to be reviewed.

Copy link
Member

@J5lx J5lx left a comment

Choose a reason for hiding this comment

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

LGTM, please merge at your own discretion. Thank you for taking care of this!

app/src/colorpalettewidget.cpp Outdated Show resolved Hide resolved
@MrStevns MrStevns added this to the v0.6.7 milestone Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Color Palette 🔹 Minor PR (only one reviewer required)
Projects
Status: No status
5 participants