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

Multiple spectrum indicators 4338 #4340

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Charlie-83
Copy link
Contributor

@Charlie-83 Charlie-83 commented Feb 25, 2024

#4338

I agree that my contributions are licensed under the Individual Contributor License Agreement V4.0 ("CLA") as stated in https://github.com/igarastudio/cla/blob/main/cla.md

I have signed the CLA following the steps given in https://github.com/igarastudio/cla#signing

image

This is super rough right now but opening PR to gauge interest in this feature.

TODO:

  • Multiple indicators for bottom bars
    • Saturation bar
    • Alpha bar
  • Saturation bar indicators should all be black or all be white
  • Have some way of showing which indicator in the main area corresponds to which indicator in the bottom bar. I'm thinking either having each indicator be a different shape/colour or drawing a dotted line between the connected indicators.
  • Multiple indicators for other colour selectors
  • Probably want to store the shade as m_shade on the colour selector
  • Render the spectrum using the maximum saturation among selected colours

@Charlie-83 Charlie-83 marked this pull request as draft February 25, 2024 09:04
@dacap dacap self-assigned this Mar 5, 2024
@dacap
Copy link
Member

dacap commented Mar 5, 2024

Thanks for this PR @Charlie-83, I've been testing this, and it looks a little confusing seeing all the indicators at the same time (with the same color).

Not sure if it's useful (probably it could be a switchable option, off by default), Probably seeing the indicators with a some alpha blending (50%) might be useful to differentiate between the current indicator and the other colors indicators, or avoid showing indicators for all colors and just show a dotted line connecting all points (and show the indicator only in the current color).

@Charlie-83
Copy link
Contributor Author

Charlie-83 commented Mar 5, 2024

Thanks for having a look @dacap I really like the idea of showing one indicator and then a dotted line for the rest since the main point of the feature is to be able to visualize color curves (although that's gonna be harder to do since the logic for drawing the indicators was already there).

This is currently just a rough hack. I'll find some time to work on it some more.

Copy link
Collaborator

@aseprite-bot aseprite-bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/app/ui/color_selector.h Outdated Show resolved Hide resolved
src/app/ui/color_selector.h Outdated Show resolved Hide resolved
src/app/ui/color_spectrum.cpp Outdated Show resolved Hide resolved
src/app/ui/color_spectrum.cpp Outdated Show resolved Hide resolved
src/app/ui/color_spectrum.cpp Outdated Show resolved Hide resolved
src/app/ui/color_spectrum.cpp Outdated Show resolved Hide resolved
src/app/ui/color_spectrum.cpp Outdated Show resolved Hide resolved
src/app/ui/color_spectrum.cpp Outdated Show resolved Hide resolved
src/app/ui/color_spectrum.cpp Outdated Show resolved Hide resolved
src/app/ui/color_spectrum.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@aseprite-bot aseprite-bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/app/ui/color_selector.cpp Show resolved Hide resolved
src/app/ui/color_selector.cpp Show resolved Hide resolved
src/app/ui/color_selector.cpp Outdated Show resolved Hide resolved
src/app/ui/color_selector.h Outdated Show resolved Hide resolved
src/app/ui/color_selector.h Outdated Show resolved Hide resolved
src/app/ui/color_spectrum.cpp Outdated Show resolved Hide resolved
@Charlie-83 Charlie-83 force-pushed the multiple-spectrum-indicators-4338 branch 2 times, most recently from 1590bcb to 6f35ea5 Compare March 7, 2024 08:55
Copy link
Collaborator

@aseprite-bot aseprite-bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/app/ui/color_selector.cpp Show resolved Hide resolved
src/app/ui/color_spectrum.cpp Show resolved Hide resolved
@Charlie-83 Charlie-83 force-pushed the multiple-spectrum-indicators-4338 branch from 6f35ea5 to c11cf7e Compare March 7, 2024 09:06
@Charlie-83
Copy link
Contributor Author

image
Here is what I have it looking like now

@dacap
Copy link
Member

dacap commented Mar 12, 2024

I was doing some testing and I think we are getting closer. I'm not 100% convinced to enable this by default, but can be useful for some users.

Some issues I've found:

  1. Probably it would make sense to draw a line between the near hue distance:

Screenshot from 2024-03-12 16-47-51

  1. Sometimes the line is not visible

Screenshot from 2024-03-12 16-47-29

I didn't review the code yet.

@Charlie-83
Copy link
Contributor Author

I'll fix those issues. I agree that this is probably best as an option in the settings default off so I'll work on adding a setting for this too

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

3 participants