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

Improved color generator and fix #300 #306

Closed
wants to merge 1 commit into from

Conversation

skjnldsv
Copy link
Contributor

Better colors randomisation.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @Henni to be a potential reviewer

@codecov-io
Copy link

Current coverage is 10.76%

Merging #306 into master will decrease coverage by -0.02% as of 2d97b3e

@@            master    #306   diff @@
======================================
  Files           40      40       
  Stmts          714     715     +1
  Branches         0       0       
  Methods          0       0       
======================================
  Hit             77      77       
  Partial          0       0       
- Missed         637     638     +1

Review entire Coverage Diff as of 2d97b3e


Uncovered Suggestions

  1. +2.10% via ...ntactColor_filter.js#20...34
  2. +2.10% via ...dressBook_service.js#148...162
  3. +1.82% via ...ntactColor_filter.js#3...15
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

@DeepDiver1975
Copy link
Member

Please add Screenshots before and after. THX a lot

@skjnldsv
Copy link
Contributor Author

Well, I changed my mind, I was exploring a new way to improve the color generator, but this doesn't seems good enough.
The current algorithm displays poor data repartition: (range is 0-360)
image 1

I tried working a second one, but can't figure out some issues in the data:
image 2
image 3

@skjnldsv
Copy link
Contributor Author

Okay, I managed to get something much more usable:
image 5

@DeepDiver1975 The colors are the same, I just changed the way the script transform the hash into a 0-360 int, so it has better random "distribution"

@skjnldsv skjnldsv changed the title Improved color generator Improved color generator and fix #300 Mar 27, 2016
@Henni
Copy link
Contributor

Henni commented Mar 27, 2016

I like these graphs!
@skjnldsv could you please document your thoughts/algorithm in the code

@skjnldsv
Copy link
Contributor Author

Hum, I can try @Henni! English not my main language but whatever! :)

The uuid system uses md5 which already is correctly distributed see here, someone already did the job
The main goal was figuring out an algorithm which isn't heavy and gives us data that are evenly distributed between 0 and 360 (deg for the colours wheel).

At first i tried separating every char of the hash and converting form base16 to base10. Adding it up alltogether and dividing by the max we could get (32*16, md5 has 32char).
But because it's a sum, we have a function who converge in the medium values (see first graph).
So I came up with the idea to divise the hash in part, and remove the sum. (the modulo part on the script)

I decided to set up a formula that isn't mathematically logic, because I don't sum the base16 to 10. I convert the result to string and concatenate them, so 15+16 = 1516 and not 31.
The repartition overall should be even because all characters within the hash are 0-9a-f so we can have a statistically correct repartition.
BUT, I still needed to convert my hash division sum into a color. So to smooth the data even more, I decided to add the previous hash division results up within a modulo 3 that i could distribute to 3 variables: red, green and blue.

Then I just had to use the formula to convert rgb to hsl and retrieve the hue variable from the hsl.
Here is a beautiful paint to resume my terrible explanation! (The first letters are not in order, the characters position in the following graph are 1, 17, 2, 18, 3, 19, 4, 20.....)
fer

@skjnldsv
Copy link
Contributor Author

I'm sure the algorithm can be improved, but hey, do we really need a perfect colour generator? X)
I love working on those kinds of functions, but I'm not convinced this is vital ^^

New generator based on the same color tones

Fix #300
Only keep numbers and a-f letters used in uids
@skjnldsv
Copy link
Contributor Author

We should probably cancel this PR and wait for this to be merged: owncloud/core#23638

After we should update our templates to use the element.imageplaceholder(hash,name) function to generate our avatars

@DeepDiver1975 DeepDiver1975 added this to the 1.2 milestone Mar 30, 2016
@DeepDiver1975
Copy link
Member

unit tests would be great to improve code coverage

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Apr 3, 2016

I'm closing this because we need to use the core function.
refs owncloud/core#23638

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants