joncardasis / ChromaColorPicker

:art: An intuitive iOS color picker built in Swift.
MIT License
569 stars 127 forks source link

AdjustToColor does not update shade slider #17

Closed joris416 closed 5 years ago

joris416 commented 7 years ago

Hi,

When call the adjustToColor method on your colorPicker, it does adjust to the color I picked in the circular picker, but not the shade slider, which results in another color being loaded in the picker than I intended. Would it be possible to create a workaround for this?

florentalexdr commented 7 years ago

Same issue, would love a workaround.

joncardasis commented 7 years ago

There may be a possible solution. I'd have to run a chosen color through some math to find a point on the shade slider between 0 and 1 for its saturation component. I'll take a look at this one within the next few days

ashleymills commented 7 years ago

Hi - any news on this issue?

joncardasis commented 7 years ago

I've unfortunately been very busy with a move and new position. This, as well as an update for Swift 4, are my top OSS priorities when I get the chance. It may be another week until I have an update. Feel free to implement and create a pull request if you need an update sooner :).

ManraajNijjar commented 7 years ago

Hey, I'm working on a potential fix but I was curious on how you would want the ColorPicker to work after a switch to a color not in it's range. Would you rather it stay the slider maintain it's normal range

screen shot 2017-10-03 at 8 12 01 pm

Or switch to match the new color

screen shot 2017-10-03 at 8 10 06 pm

Also my fix doesn't work quite just yet, I've still got a bit of wonkiness on the math to handle. I'm new to trying to handle color values

joncardasis commented 7 years ago

Off the top of my head the structure would need to go something like this:

Given a UIColor to update for, let's call it c

  1. Update handle: c -> HSV representation -> use the angleForColor: method to get the angle the control handle will need to update to.
  2. Update shade slider: I believe I had this mostly rely on saturation. Therefore, the saturation value of c (between 0.0 and 255.0) should map to a value between the slider range (-1, 1) (inclusive). From there the currentValue attribute would be set and then layoutLayerFrames would be called.

That's the general idea at least. If you find a solution please post your findings. Unfortunately, I haven't had the time to maintain this project as well as I have wanted to previously.

aspyre commented 7 years ago

@ManraajNijjar if you get a chance to share you solution, it would be very much appreciated!

ManraajNijjar commented 7 years ago

@aspyre I made a pull request with my solution

aspyre commented 6 years ago

Thanks @ManraajNijjar, not having much luck with your solution unfortunately. It seems to end up modifying the shade slider for just about all colours, as I guess most of the time brightness & saturation is less than 1.0.

@joncardasis I gave your off the top of your head structure a go, but from what I can tell this basically just boils down to

shadeSlider.currentValue = saturation * 2 - 1

Which doesn't have the desired result. If you get a chance to take a look or provide some more direction, it would be fantastic. Appreciate you're busy with your new position though, congrats!

joncardasis commented 6 years ago

@aspyre The boiled down solution does seem right as that would map the values to [-1,1] – at least for updating the value on the shadeSlider.

This project definitely needs some TLC in areas and I have it on my ToDo list for this week after other project deadlines wrap up and I can breathe a sigh of relief :P.

The idea would be to map every color to its closest possible location on the wheel (which is based solely on the hue component of the color) and then have the saturation and brightness get mapped to the shadeSlider. Hue shouldn't be an issue...

Diving in a bit, I don't know if the project could currently support this feature. The wheel is based on hue and the shadeSlider based on brightness. It assumes a saturation value of 1.0 — always. Because of this the control currently has no support for saturations. You can't actually select a value with less than 1.0 saturation. While I could have it update the slider to match any given UIColor, once you move the slider again it will assume 1.0 saturation again.

Sorry for the long-winded post, but the first idea that comes to my head is to rework the ChromaShadeSlider and abstract it into ChromaSlider which a renamed ChromaBrightnessSlider and a new ChromaSaturationSlider would inherit from. So the control may have two sliders under it to then support every color and the bubble in the upper left could then be removed since grays and whites could then be possible. Quite an undertaking, however, at which point I may be interested in creating an entire new project as to not disturb and current utilization.

aspyre commented 6 years ago

Thanks for the detail @joncardasis, certainly interesting to hear. I thinking mapping to the closest possible colour in general makes sense - adding a second slider would potentially be a good thing to cover all the cases, but also complicate things from a UI perspective.

As an example of a color I have trouble with at the moment, F1F1F1 has no actual saturation (or hue), just a brightness of 95. So I would think that particular color should actually be ok, but it's a bit above me as to why it doesn't.

Anyway - good luck with your other project deadlines :)

joncardasis commented 5 years ago

I am starting a rewrite of a number of features on this project for better customizability. If you have any suggestions you'd like to see in the evolution of this project, please open a new issue with the EVOLUTION REQUEST label.