From ae89135fd642d0535bf5826b95df7f445c92cf28 Mon Sep 17 00:00:00 2001 From: Scott Vorthmann Date: Mon, 2 Sep 2024 12:44:03 -0700 Subject: [PATCH 1/2] Use ColorManagement better when constructing Colors Solid-three will never set a color without using ColorManagement.workingColorSpace, or a color space passed to us explicitly. Mainly, we want to avoid Color setters that default to SRGB, so the user has better control. --- .vscode/launch.json | 15 +++++++++++++++ playground/App.tsx | 1 - playground/Box.tsx | 7 +++++-- src/canvas.tsx | 2 +- src/props.ts | 29 +++++++++++++++++++++++++---- 5 files changed, 46 insertions(+), 8 deletions(-) create mode 100644 .vscode/launch.json diff --git a/.vscode/launch.json b/.vscode/launch.json new file mode 100644 index 0000000..95445ed --- /dev/null +++ b/.vscode/launch.json @@ -0,0 +1,15 @@ +{ + // Use IntelliSense to learn about possible attributes. + // Hover to view descriptions of existing attributes. + // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387 + "version": "0.2.0", + "configurations": [ + { + "type": "chrome", + "request": "launch", + "name": "Launch Chrome against localhost", + "url": "http://localhost:5173", + "webRoot": "${workspaceFolder}" + } + ] +} \ No newline at end of file diff --git a/playground/App.tsx b/playground/App.tsx index f084c7f..6003eae 100644 --- a/playground/App.tsx +++ b/playground/App.tsx @@ -8,7 +8,6 @@ import "./index.css"; extend(THREE); export const App: Component = () => { - // return "hallo"; return ( diff --git a/playground/Box.tsx b/playground/Box.tsx index ea1e22a..776a0c3 100644 --- a/playground/Box.tsx +++ b/playground/Box.tsx @@ -1,5 +1,5 @@ import { createSignal } from "solid-js"; -import { Mesh } from "three"; +import { Color, Mesh, SRGBColorSpace } from "three"; import { T, useFrame } from "../src"; export function Box() { @@ -8,6 +8,9 @@ export function Box() { useFrame(() => (mesh!.rotation.y += 0.01)); + const green = new Color(); + green .setStyle( "green", SRGBColorSpace ); + return ( <> setHovered(false)} > - + ); diff --git a/src/canvas.tsx b/src/canvas.tsx index 841be74..d4dcb3a 100644 --- a/src/canvas.tsx +++ b/src/canvas.tsx @@ -77,7 +77,7 @@ export interface CanvasProps extends ComponentProps<"div"> { * @returns A div element containing the WebGL canvas configured to occupy the full available space. */ export function Canvas(_props: CanvasProps) { - const [props, canvasProps] = splitProps(_props, ["fallback", "camera", "children", "ref"]); + const [props, canvasProps] = splitProps(_props, ["fallback", "camera", "children", "ref", "linear"]); let canvas: HTMLCanvasElement; let container: HTMLDivElement; diff --git a/src/props.ts b/src/props.ts index 9a22199..4a297d3 100644 --- a/src/props.ts +++ b/src/props.ts @@ -10,6 +10,7 @@ import { import { BufferGeometry, Color, + LinearSRGBColorSpace, Fog, Layers, Material, @@ -17,6 +18,7 @@ import { RGBAFormat, Texture, UnsignedByteType, + ColorManagement, } from "three"; import { S3 } from "./"; import { $S3C } from "./augment"; @@ -163,7 +165,7 @@ export const applyProp = (source: S3.Instance, type: string, value: any) = const canvasProps = useCanvasProps(); try { - // Copy if properties match signatures + // Copy if properties match signatures. if (target?.copy && target?.constructor === value?.constructor) { target.copy(value); } @@ -171,6 +173,24 @@ export const applyProp = (source: S3.Instance, type: string, value: any) = else if (target instanceof Layers && value instanceof Layers) { target.mask = value.mask; } + else if ( target instanceof Color ) { + // value is NOT a Color (would have taken the "Copy" branch) + if ( Array.isArray(value) ) { + if ( value.length == 2 ) { + target .setStyle( ...value ); + } else if ( value.length == 4 ) { + target .setRGB( ...value ); + } else if ( value.length == 3 ) { + target .setRGB( ...value, ColorManagement.workingColorSpace ); + } else { + console.error( `Color properties cannot be set from length-${value.length} arrays` ); + } + } else if ( typeof value === 'string' ) { + target .setStyle( value, ColorManagement.workingColorSpace ); + } else { + console.error( `Color properties cannot be set from values of type ${typeof value}` ); + } + } // Set array types else if (target?.set && Array.isArray(value)) { if (target.fromArray) target.fromArray(value); @@ -179,11 +199,12 @@ export const applyProp = (source: S3.Instance, type: string, value: any) = // Set literal types, ignore undefined // https://github.com/pmndrs/react-three-fiber/issues/274 else if (target?.set && typeof value !== "object") { - const isColor = target instanceof Color; // Allow setting array scalars - if (!isColor && target.setScalar && typeof value === "number") target.setScalar(value); + if ( target.setScalar && typeof value === "number" ) target.setScalar(value); // Otherwise just set ... - else if (value !== undefined) target.set(value); + else if (value !== undefined) { + target.set(value); + } } // Else, just overwrite the value else { From 538f5ef5955abf796d77df2d98c78440fd8fd9be Mon Sep 17 00:00:00 2001 From: Scott Vorthmann Date: Mon, 9 Sep 2024 11:41:27 -0700 Subject: [PATCH 2/2] Reversed prior approach to color management After discussion, we concluded that the declarative style should use the Three.js defaults, even if we doubt their approach. Users of `solid-three` can always set colors imperatively if they want to have precise control. --- playground/App.tsx | 5 ++++- playground/Box.tsx | 8 ++++---- src/props.ts | 18 ------------------ 3 files changed, 8 insertions(+), 23 deletions(-) diff --git a/playground/App.tsx b/playground/App.tsx index 6003eae..a86b7f5 100644 --- a/playground/App.tsx +++ b/playground/App.tsx @@ -8,9 +8,12 @@ import "./index.css"; extend(THREE); export const App: Component = () => { + + const ambientColor = new THREE.Color() .setRGB( 0.4, 0.4, 0.4, THREE.LinearSRGBColorSpace ); + return ( - + diff --git a/playground/Box.tsx b/playground/Box.tsx index 776a0c3..eef58a7 100644 --- a/playground/Box.tsx +++ b/playground/Box.tsx @@ -1,5 +1,5 @@ import { createSignal } from "solid-js"; -import { Color, Mesh, SRGBColorSpace } from "three"; +import { Color, LinearSRGBColorSpace, Mesh } from "three"; import { T, useFrame } from "../src"; export function Box() { @@ -8,8 +8,8 @@ export function Box() { useFrame(() => (mesh!.rotation.y += 0.01)); - const green = new Color(); - green .setStyle( "green", SRGBColorSpace ); + const green = new Color() .setStyle( "green", LinearSRGBColorSpace ); + const red = new Color() .setStyle( "red", LinearSRGBColorSpace ); return ( <> @@ -19,7 +19,7 @@ export function Box() { onPointerLeave={e => setHovered(false)} > - + ); diff --git a/src/props.ts b/src/props.ts index 4a297d3..0daa39a 100644 --- a/src/props.ts +++ b/src/props.ts @@ -173,24 +173,6 @@ export const applyProp = (source: S3.Instance, type: string, value: any) = else if (target instanceof Layers && value instanceof Layers) { target.mask = value.mask; } - else if ( target instanceof Color ) { - // value is NOT a Color (would have taken the "Copy" branch) - if ( Array.isArray(value) ) { - if ( value.length == 2 ) { - target .setStyle( ...value ); - } else if ( value.length == 4 ) { - target .setRGB( ...value ); - } else if ( value.length == 3 ) { - target .setRGB( ...value, ColorManagement.workingColorSpace ); - } else { - console.error( `Color properties cannot be set from length-${value.length} arrays` ); - } - } else if ( typeof value === 'string' ) { - target .setStyle( value, ColorManagement.workingColorSpace ); - } else { - console.error( `Color properties cannot be set from values of type ${typeof value}` ); - } - } // Set array types else if (target?.set && Array.isArray(value)) { if (target.fromArray) target.fromArray(value);