-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
Examples: Updates for color management (pt3) #25889
Changes from 9 commits
fb742f4
7abf594
feffd85
cfc13cc
2697198
e67b098
acdca7f
a26a089
4531966
c656a4a
b0afcf2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @takahirox does this result seem acceptable for the Hatsune Miku character? |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,8 +32,6 @@ | |
import { OrbitControls } from 'three/addons/controls/OrbitControls.js'; | ||
import { KMZLoader } from 'three/addons/loaders/KMZLoader.js'; | ||
|
||
THREE.ColorManagement.enabled = false; // TODO: Consider enabling color management. | ||
|
||
let camera, scene, renderer; | ||
|
||
init(); | ||
|
@@ -55,7 +53,7 @@ | |
|
||
scene.add( camera ); | ||
|
||
const grid = new THREE.GridHelper( 50, 50, 0xffffff, 0x333333 ); | ||
const grid = new THREE.GridHelper( 50, 50, 0xffffff, 0x7b7b7b ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The default color values of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question! Replied in main thread, since it is an important question and I don't want to lose the discussion. |
||
scene.add( grid ); | ||
|
||
renderer = new THREE.WebGLRenderer( { antialias: true } ); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm currently reviewing the remaining explicit usage of
ColorManagement
in the example code.Do we need this check here? This is the only loader which conditionally performs a color space conversion based on
ColorManagement.enabled
. Can't we just always assume color management and sRGB output is enabled?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the check would also be fine to remove, yes. VOXLoader does something similar:
three.js/examples/jsm/loaders/VOXLoader.js
Line 199 in 673eca6