(reverted) feat(core): default physicallyCorrectLights to true
#2148
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 4d0ae2f:
|
gsimone
reviewed
Mar 25, 2022
packages/fiber/src/core/index.tsx
Outdated
| @@ -179,6 +179,7 @@ function createRoot<TCanvas extends Element>(canvas: TCanvas): ReconcilerRoot<TC | |||
| const toneMapping = flat ? THREE.NoToneMapping : THREE.ACESFilmicToneMapping | |||
| if (gl.outputEncoding !== outputEncoding) gl.outputEncoding = outputEncoding | |||
| if (gl.toneMapping !== toneMapping) gl.toneMapping = toneMapping | |||
| if (!gl.physicallyCorrectLights) gl.physicallyCorrectLights = true | |||
There was a problem hiding this comment.
Shouldn't this check for undefined instead? So that it defaults to true only when undefined, otherwise it would always end up true (eg.
gl={{
physicallyCorrectLights: false
}}
There was a problem hiding this comment.
I agree, it's also more inline with the above statements.
There was a problem hiding this comment.
gl.physicallyCorrectLights will always be false -- that's WebGLRenderer here. We apply glOptions afterward which corresponds to the Canvas gl prop.
I'll make it more inline with the other options as far as code style but I worry the behavior can deviate. I've updated related tests to harden this area as the nature of root#configure makes this more error-prone.
CodyJasonBennett
force-pushed
the
physical-lights
branch
from
cc8d8b4
to
40609ad
Compare
|
@drcmda, I've updated the related docs so we can merge and release. |
|
thanks @CodyJasonBennett ! |
|
Reverting on the v8 branch, this is quite an involved change for users. I'd like to see this further discussed and treated behind the scenes like color management. |
drcmda
added a commit
that referenced
this pull request
Mar 30, 2022
* react 18 alpha * up use-asset * fix flat * silence disposal errors * support instanced event cancelation * simplify useFrame * Allow partial WebGLRenderer params (#1656) Without the `Partial` the `gl` props is practically unusable as it requires all the params to be present in the prop. * is.test: 'is equal' test to use alredy declared obj and array. (#1643) * refactor into utility functions * refactor * bring over diffprops bug * Merge branch 'master' into v8 * chore: move dpr calc to utils * chore: don't use `is` outside of renderer * fix(core): set null parent in prepare * fix args in createinstance * fix primitve leftovers on switch, add gltf example * chore(web): fix mangled import * make applyprops return the instance * fix merge * spread props over container div, change dpr default, fix createRoot * export createroot * fix cherrypick * [v8] feat: add react-native support (#1699) * chore: add native target from: #1384 Co-authored by @swittk. * fix: set initial render size * refactor: cleanup render API, remove shim * chore: cleanup canvas, TS * chore: map events TODO: bind them * refactor: move expo to Canvas, allow custom renderers * chore: add RN types * refactor: accept constructor args in canvas * chore: bind events * chore: synchronously get handlers from target * chore: remove dependency on expo-three * chore: cleanup Canvas TS * chore: create native entrypoint * chore: cleanup context init * refactor: accept context over canvas * fix: passthrough gl props * chore: remove entrypoint This is created automatically but we'll have to update the CI to skip it. * chore: update canvas props TS * chore: fix native exports * chore: hide __mocks__ from npm output * chore: add TS to native entrypoint * chore: cleanup context TS * chore: update native recipe * chore: add back in bootstrap tip for native TODO: update https://github.com/expo/examples/tree/master/with-react-three-fiber * chore: mirror dpr/size fix from #1707 * feat: add v5 native props * chore: add expo-gl as a dependency * chore: don't use `is` outside of renderer * feat(native): export native useLoader * fix(native/hooks): respect module paths, fix constructor compar * chore(native): streamline loader TS, mirror GLProps from web * chore(native): defer to RenderProps for GL * docs: remove expo-gl dep, use same API * chore: delete RN key in package * chore: bump native deps * chore(native): pin dpr to screen dpr * fix(native): set color management defaults * Merge branch 'v8' into native * chore(Canvas): accept native props * chore(native): export createRoot * fix(Canvas): pass onCreated * [v8] feat: remove vr prop for dynamic render switching (#1702) * fix(Canvas): pass events & size * feat: add `renderVisible` prop to only render while in view * remove zustand sub selector * feat(core): make `frameloop` reactive * chore: `renderVisible` => `include` * chore: add demo for IncludeProp * docs: add `intersect` prop and internal `setFrameloop` * chore: cleanup example with styled-components * chore: revert `intersect` for native * chore: fix peer dep range for native * [v8] refactor(core): use-asset => suspend-react (#1797) * refactor(core): use-asset => suspend-react * fix(native/hooks): use correct import * fix(core): fix shallow equality in useLoader * chore(hooks): fix TS * fix: use correct keys syntax * chore: remove `intersect` prop from web & docs, example * chore: add react-native key for lib interop * update deps * update version * chore: fix native peer dep * chore: update references from github namespace react-spring to pmndrs * fix: tree-shaking * fix: revert removing mergerefs, use pure annotations instead * align both canvases * update usemeasure * Fix missing type for Float32BufferAttribute (#1862) Missing Float32BufferAttriute type causes Typescript error "Property 'float32BufferAttribute' does not exist on type 'JSX.IntrinsicElements'.ts(2339)" Example of use of float32BufferAttribute ```jsx <instancedBufferGeometry attach="geometry" instanceCount={count}> <float32BufferAttribute attachObject={['attributes', 'position']} args={[positionArray, 3]} /> <instancedBufferAttribute attachObject={['attributes', 'offset']} args={[offsetArray, 3]} /> </instancedBufferGeometry> ``` * fix(Canvas): unmount current context * [v8] fix: use new act API & fix extend for tests (#1891) * feat(test-renderer): add RegExp matcher support to `findByProps` and `findAllByProps` (#1890) * chore: apply fix to applyProps * fix: schedule child.dispose() (#1887) * chore: fix eslint * add camera:manual * remove console log * move up to react rc * update examples to react-router 6 * fix three types * unify attach * fix suspense re-attach * chore: update tests * fix(types): use new attach API * switch to createRoot * chore: update test-renderer snapshot * publish new beta * fix(RTTR): use createRoot * chore(tests): use createRoot * fix(web): render when canvas is created * refactor: move createRoot to core (#1915) * chore(native): add expo unimodules as dep (#1916) * clean up example * up version * chore(tests): add native coverage, sort tests (#1917) * fix(native): revert to Pressability (#1923) * fix(native): use relative coords for events * fix(core): safely allocate `now` (#1959) * chore: update new tests to v8 API * fix(native): use expo modules (#1927) * chore: update docs for v8 (#1976) * experiment: unify hooks (#1994) * fix(native): unpin and correctly mark peer deps (#2004) * fix(native): unpin and correctly mark peer deps * chore: add expo-asset as dev dep * feat(core): pass XRFrame to subscribers (#2017) * fix(native): move expo-gl to peer dep, update react-native instructions/example (#2033) * up package * fix: remove unused resize-observer-polyfill dep (#2044) * chore(native): export instance types * docs(changeset): new beta for library testing * RELEASING: Releasing 2 package(s) Releases: @react-three/[email protected] @react-three/[email protected] [skip ci] * v8.0.0-beta.5 * root.configure() * fix(configure): only configure shadowmap if present * fix(native): prefer to resolve external assets * cleanup refactor, extend is.equ * perf adjustments to the renderloop * release new beta * fix(types): add Object3D to IntrinsicElements (#2099) * fix(types): add more bufferAttribute intrinsic elements (#2102) * rc1 * rc * fix test * fix(types): move @types/react-reconciler to dependencies (#2126) * introduce the inject native element, which creates a layered host context * new beta * remove host context, make one up with portals * inject example * use userdata instead of __context * chore: update instance snapshots for new args handling * inject cleanup * example: update `MultiMaterial` example (#2144) * fix(attach): add back `attachArray` * chore: revert code and update example * chore: revert code Co-authored-by: Cody Bennett <[email protected]> * fix: is.equ and attach arrays * remove arrayed attach, prefer function with cleanup * fix: events were broken due to is.equ * enforce attach fn cleanup * refactor portal context * add inject layer for portals, expose it, too * completely revamp portals * cleanup * refactor(core): create an array when indexing with attach (#2145) * refactor(core): create an array when indexing with attach * chore: cleanup attach exp * make 139 peer, remove color correction * update snapshots * fix(Canvas): memoize `onPointerMissed` (#2146) * chore: changes to examples & local linking (#2151) * chore: changes to examples & local linking * . * . * . * minor changes * minor changes * removes bright backgrounds * . * removes unused stuff * removes unused stuff * . * . * . * . * . * . * . * . * work on viewcube * fix colors * chore: fixes vite config (#2152) * fix types * portal inject layers * constrain inject state to a subset, auto increase priority * demo cleanup * new compute api * multi view demo (using skissor) * fix clearcolor * connect events to the canvas parent instead of the canvas itself * cleanup * instead of hijacking the store, make up a new one in createportal * new beta * chore(demos): cleanup render effects Nits from #2153 * chore(tests): de-escalate react-dom/client warnings * fix: tests, add parent property * fix: native canvas registers its parent * chore(tests): fix react-dom silence * chore(tests): remove react-dom/client workaround * chore(tests): move console mock to setuptests * chore(tests): UseStore => UseBoundStore * fix: remove parent * fix: black list for inject layer props * chore(docs): update createRoot signature * chore(docs): add notes on webxr useframe signature * chore(docs): streamline render methods (#2162) * feat(core): default `physicallyCorrectLights` to true (#2148) * feat(core): default `physicallyCorrectLights` to true * chore: streamline gl defaults * chore(docs): note new gl defaults * chore(tests): flip gl prop case, respect lighting preferences * mm as the default example * chore: revert physicallyCorrectLights * deprecate mouse, setevents, more inline docs * forgot partial * update all dependencies * clean up demos * add missing migration parts * add missing migration parts * fix: applyProps was supposed to return the instance * migration * last beta * migration example Co-authored-by: Dušan Maliarik <[email protected]> Co-authored-by: Anderson Leite <[email protected]> Co-authored-by: Cody Bennett <[email protected]> Co-authored-by: Cody Bennett <[email protected]> Co-authored-by: dangrabcad <[email protected]> Co-authored-by: davidblnc <[email protected]> Co-authored-by: Karol Stopyra <[email protected]> Co-authored-by: Egor Blinov <[email protected]> Co-authored-by: Alexander Nanberg <[email protected]> Co-authored-by: Josh <[email protected]> Co-authored-by: Nathan Bierema <[email protected]> Co-authored-by: susiwen8 <[email protected]> Co-authored-by: Gianmarco <[email protected]>
CodyJasonBennett
changed the title
physicallyCorrectLights to truephysicallyCorrectLights to true
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Enables
WebGLRenderer.physicallyCorrectLightsto respect PBR workflows (see #2127). Can be disabled (or enabled on v7) viagl={{ physicallyCorrectLights: false }}.Internally, this multiplies lights' intensities by a factor of 1 instead of PI (among other things for pointlights, see #2127 (comment)).
@drcmda, should we couple this prop with
flat?