Make CanvasRenderer treat clearColor as RGBA, like WebGlRenderer - #869
Make CanvasRenderer treat clearColor as RGBA, like WebGlRenderer#869guilhermesimoes wants to merge 5 commits into
CanvasRenderer treat clearColor as RGBA, like WebGlRenderer#869Conversation
| const parsedRgbaColors: Map<number, string> = new Map(); | ||
|
|
||
| export function normalizeCanvasColor(color: number, isRGBA: boolean = false) { | ||
| export function normalizeCanvasColor(color: number, isRGBA: boolean = true) { |
There was a problem hiding this comment.
Not sure if you agree with the second commit that inverts this default.
If you don't agree I can undo this change.
There was a problem hiding this comment.
Btw, I also thought about splitting this function into two: normalizeCanvasColor and normalizeCanvasColorArgb. I generally subscribe to the idea of avoiding boolean params in functions. Tell me what you think.
There was a problem hiding this comment.
I'm much more in favor of splitting it in two sep functions! Since it already gets the value from 2 distinct caches and two distinct parseTo<variant>String functions, it would be cheaper to make 2 explicit functions for this.
I would also immediately inline the parseToRgbaString / parsedArgbColors functions to avoid the additional function hop. And remove them from utils, no one else but this function seems to be using them.
Great suggestion and love the article!
There was a problem hiding this comment.
I split the functions into two.
The utils parseTo<variant>String would actually be useful to us if they were exported. I noticed we have our own implementations of these.
3fc42a6 to
d97e768
Compare
| const parsedRgbaColors: Map<number, string> = new Map(); | ||
|
|
||
| export function normalizeCanvasColor(color: number, isRGBA: boolean = false) { | ||
| export function normalizeCanvasColor(color: number, isRGBA: boolean = true) { |
There was a problem hiding this comment.
I'm much more in favor of splitting it in two sep functions! Since it already gets the value from 2 distinct caches and two distinct parseTo<variant>String functions, it would be cheaper to make 2 explicit functions for this.
I would also immediately inline the parseToRgbaString / parsedArgbColors functions to avoid the additional function hop. And remove them from utils, no one else but this function seems to be using them.
Great suggestion and love the article!
Applying & 0xff twice is a no-op. The second mask does nothing since the value is already clamped to 8 bits.
Destructuring creates an intermediate object scope, avoid it.
| const b = (abgr >>> 16) & 0xff & 0xff; | ||
| const g = (abgr >>> 8) & 0xff & 0xff; | ||
| const r = abgr & 0xff & 0xff; |
There was a problem hiding this comment.
Is this code correct?
The variable is named abgr and the variables are a, b, g, r.
Shouldn't it be argb and the variables be a, r, g, b?
There was a problem hiding this comment.
the variable is probably off - but the logic below works, its just the declaration of the incoming number.
There was a problem hiding this comment.
You sure it's correct? Shouldn't the r and the b be swapped? Like so
const a = ((argb >>> 24) & 0xff) / 255;
const r = (argb >>> 16) & 0xff;
const g = (argb >>> 8) & 0xff;
const b = argb & 0xff;There was a problem hiding this comment.
yeah for making it look better, for sure!
| const b = (abgr >>> 16) & 0xff & 0xff; | ||
| const g = (abgr >>> 8) & 0xff & 0xff; | ||
| const r = abgr & 0xff & 0xff; |
There was a problem hiding this comment.
the variable is probably off - but the logic below works, its just the declaration of the incoming number.
| } | ||
| targetCache.set(color, out); | ||
|
|
||
| out = parseToAbgrString(argbColor); |
There was a problem hiding this comment.
small nit picky thing - instead of calling parseToAbgrString here
can you just inline/copy the entire contents of the function here? no need to do the function call hop and object creation when this is the only place that uses the function.
| out = parseToRgbaString(color); | ||
| } else { | ||
| out = parseToAbgrString(color); | ||
| out = parseToRgbaString(rgbaColor); |
There was a problem hiding this comment.
small nit picky thing - instead of calling parseToRgbaString here
can you just inline/copy the entire contents of the function here? no need to do the function call hop and object creation when this is the only place that uses the function.
| return { isWhite: false, a, r, g, b }; | ||
| } | ||
|
|
||
| export function parseToAbgrString(abgr: number): string { |
There was a problem hiding this comment.
we should remove this from colorParser.ts and inline in the above function.
| return `rgba(${r},${g},${b},${a})`; | ||
| } | ||
|
|
||
| export function parseToRgbaString(rgba: number): string { |
There was a problem hiding this comment.
same here, just remove this entire function and inline it above.
2 participants
Currently if we do
stage.setClearColor(0x000000FF)in WebGL the background is black, but in Canvas it's white (because it thinks we're using ARGB instead of RGBA). Let's change this so that both WebGL and Canvas are consistent. And also so thatapp.props.color = 0x000000FFis consistent withapp.stage.setClearColor(0x000000FF).