Opened 12 years ago

Closed 12 years ago

#3653 closed defect (fixed)

dojo.Color.setColor(colorObj) returns invalid object (should work?)

Reported by: guest Owned by: Eugene Lazutkin
Priority: high Milestone: 0.9
Component: General Version: 0.9
Keywords: Cc: Adam Peller
Blocked By: Blocking:

Description

It seems like passing dojo.Color.setColor a dojo.Color object should return a valid dojo.Color object in response; regardless, the object returned has a null _cache, resulting in weird responses to all calls.

Attachments (2)

color-reduce.patch (8.6 KB) - added by Adam Peller 12 years ago.
use an array as internal impl, reduce some code
color_perf.html (2.5 KB) - added by Eugene Lazutkin 12 years ago.
Profiles several pieces of the proposed dojo.Color changes. Put it in dojo/tests.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 12 years ago by Adam Peller

Owner: changed from anonymous to Eugene Lazutkin

comment:2 Changed 12 years ago by Eugene Lazutkin

Milestone: 0.9
Status: newassigned

comment:3 Changed 12 years ago by Eugene Lazutkin

Resolution: fixed
Status: assignedclosed

Fixed in [9657].

comment:4 Changed 12 years ago by Adam Peller

Resolution: fixed
Status: closedreopened

Eugene, please check out some ideas for reducing the code size. See patch, attached.

comment:5 Changed 12 years ago by Adam Peller

Cc: Adam Peller added

Changed 12 years ago by Adam Peller

Attachment: color-reduce.patch added

use an array as internal impl, reduce some code

comment:6 Changed 12 years ago by Eugene Lazutkin

Some fixes are good and sound, and I want to roll them in, but using an array as a primary data holder is a questionable move.

  1. From my conversation with Alex I got that we need to implement an object because color.r, color.a are more mnemonic (and less error-prone) than color.rgba[0], color.rgba[3].
  2. Using an array reduces performance in most cases. Namely Color.sanitize() is significantly slower, Color.toCss() is slower (both versions with or without alpha), the simulated Color.toRgb() is slower too. The slowdown comes from three primary things:
    1. Array.slice() is expensive because it allocates memory and copies the array.
    2. Short loops are slower than unrolled loops.
    3. Array.forEach() is slower than the for-loop. Again see #2 above.
  3. I understood the idea that we can get away with toRgb() and toRgba(). The latter will be much faster than the original version. But both toRgb() and toRgba() are not used in our code at all. I used toRgba() in some tests, that's all.
  4. Color.sanitize() was meant to be a public method, so user can assign stuff manually, and sanitize it. But you are right --- it can be moved to the core.
  5. The preamble of dojo.blendColors() is slower with "new" operator, because it will copy. The for-loop you proposed is faster than dojo.forEach() I use. But unrolled assignments will be even faster.
  6. The private confine() function is slower than my original sanitize. In the case of non-numeric c (e.g., "3") it will convert c to Number on every comparison. Remember that c is the raw datum, which came from a user, while high and low are 100% numeric --- we control them.
  7. dojo.colorFromRgb() was named like that because it doesn't handle a very frequent case of CSS color --- hex ("#000000"). There is a separate function for that. I understand that it becomes inconsistent in dojo.colors, where new dojo.colorFromRgb() handles HSL as well. I did it purely for performance reasons. Probably the best way to handle it is to name it differently (in dojo.colors) and re-define dojo.colorFromString() as well. Actually it was the original intent.
  8. I agree with you that dojo.colorFromArray() can be shortened.

I'll attach a file, which profiles different implementations mentioned in #2 above. Run it in FF + Firebug and see results in the console window.

Changed 12 years ago by Eugene Lazutkin

Attachment: color_perf.html added

Profiles several pieces of the proposed dojo.Color changes. Put it in dojo/tests.

comment:7 Changed 12 years ago by Eugene Lazutkin

Resolution: fixed
Status: reopenedclosed

(In [9691]) Reduced size of dojo.Color, removed almost all type checks, changed the signature of dojo.blendColors(), and updated dojo.fx accordingly, dojo.Color.sanitize() moved to the core, added dojo.colors.makeGrey(), changed tests, added tests for dojo.blendColors(). Thanks, Adam. Fixes #3653.

Note: See TracTickets for help on using tickets.