Add generic Renderer type and Color4 #741
Conversation
…in ECMAScript imports
| export default class Color4 extends Color { | ||
| constructor(r: number, g: number, b: number, a?: number); | ||
|
|
||
| set(...args: [color: Color4Representation] | [r: number, g: number, b: number, a?: number]): this; |
There was a problem hiding this comment.
It doesn't seem like Color4.set can handle a Color4 correctly. Maybe we should just stick with ColorRepresentation instead of Color4Representation here?
| @@ -255,7 +255,7 @@ export class Color { | |||
| * Copies given color. | |||
| * @param color Color to copy. | |||
| */ | |||
| copy(color: Color): this; | |||
| copy(color: this): this; | |||
There was a problem hiding this comment.
Is there a benefit to changing the parameter to this? Seems like it could still just take a Color (and Color4.copy() could take a Color4)?
There was a problem hiding this comment.
The benefit was that it wouldn't need redefinition in Color4, but I neglected to remove the definition from Color4.
There was a problem hiding this comment.
Would you prefer me to make the change and remove the definition from color4 or define in both color and color4?
There was a problem hiding this comment.
I'm fine either way, I can't think of a situation where it would make a difference.
| /** | ||
| * @default true | ||
| */ | ||
| isRenderer: boolean; |
There was a problem hiding this comment.
We usually type these as readonly since they're not supposed to be modified:
readonly isRenderer: true;
| /** | ||
| * Sets the custom opaque sort function for the RenderLists. Pass null to use the default painterSortStable function. | ||
| */ | ||
| setOpaqueSort(method: (a: any, b: any) => number): void; | ||
|
|
||
| /** | ||
| * Sets the custom transparent sort function for the RenderLists. Pass null to use the default reversePainterSortStable function. | ||
| */ | ||
| setTransparentSort(method: (a: any, b: any) => number): void; |
There was a problem hiding this comment.
Can we use unknown instead of any?
There was a problem hiding this comment.
yep, I can amend that no problem
| @@ -0,0 +1,11 @@ | |||
| import { Color, ColorRepresentation } from '../../../../src/math/Color.ts'; | |||
|
|
|||
| export type Color4Representation = ColorRepresentation | Color4; | |||
There was a problem hiding this comment.
Looks like this type is unused now.
There was a problem hiding this comment.
True, I will remove
Added type definitions for the new generic Renderer type for the WebGPU Renderer. Also added the new Color4 type referenced by Renderer.