Skip to content
This repository has been archived by the owner on May 15, 2024. It is now read-only.

[Bug] Unprecise UIColor conversion #1572

Closed
tipa opened this issue Dec 4, 2020 · 7 comments · Fixed by #1716
Closed

[Bug] Unprecise UIColor conversion #1572

tipa opened this issue Dec 4, 2020 · 7 comments · Fixed by #1716
Labels
bug Something isn't working

Comments

@tipa
Copy link
Contributor

tipa commented Dec 4, 2020

Description

The precision of the color conversion for UIColor (and perhaps also for NSColor & other platforms) can be improved.

Steps to Reproduce

  1. Show UIColorPickerViewController and enter Display P3 Hex Color : #663399
  2. Get UIColor object from UIColorPickerViewController & call ToSystemColor on it
  3. Print Color as Hex string

Expected Behavior

#663399

Actual Behavior

#653299

This is because the CGColor has these components:
Red: 101.999986320734
Green: 50.9999969601631
Blue: 153.000021278858

With the current conversion, the values are rounded down:

return Color.FromArgb((int)(alpha * 255), (int)(red * 255), (int)(green * 255), (int)(blue * 255));

I think this would be a better conversion method:

return Color.FromArgb((int)Math.Round(alpha * 255), (int)Math.Round(red * 255), (int)Math.Round(green * 255), (int)Math.Round(blue * 255)); 
@tipa tipa added the bug Something isn't working label Dec 4, 2020
@jamesmontemagno
Copy link
Collaborator

Love it :)

@tipa
Copy link
Contributor Author

tipa commented Dec 16, 2020

Am I too picky? 😀
I just think when someone really wants to set their very favorite color for something, they would enter it as HEX and when returning to the color picker they expect the same HEX color to still be there

@jamesmontemagno
Copy link
Collaborator

I think you are correct, it should output the same numbers, and we should use Math.Round... the question is do you round up or down?

@tipa
Copy link
Contributor Author

tipa commented Dec 21, 2020

Not exactly sure what you mean. Math.Round rounds to nearest, so up or down depending on the number. So that 101.999986320734 would be rounded to 102 and not to 101.
Or are you asking what should be done in a the case like 101.5?

@tipa tipa mentioned this issue Mar 5, 2021
5 tasks
@tipa
Copy link
Contributor Author

tipa commented Mar 5, 2021

@jamesmontemagno created a PR in case this makes the change more likely to be merged :)

@jamesmontemagno
Copy link
Collaborator

@tipa
Copy link
Contributor Author

tipa commented Mar 5, 2021

In my PR I used the default Math.Round without specifying a Midpoint rounding. I think the default is using AwayFromZero which is what I would have used if I had to provide that parameter

CartBlanche pushed a commit to CartBlanche/Essentials that referenced this issue Sep 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants