#865 upgrade three js legacy lights #1037
Merged
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
Only BufferGeometry is supported. Every BufferGeometry helper (e.g. BoxBufferGeometry) has been renamed to shorter name (e.g. BoxGeometry)
As it leverages many deprecated methods
replace with copy() + invert()
critical and high vulnerabilities were caused by dependencies related with Jest
Previous code was setting `_position` to an empty array. This was causing some bugs where the _position array was not filled with the proper values and remained empty.
`ALPHATEST` was previously used as a flag and a value in three.js `alphatest_fragment` chunk. It was superseded by the `USE_ALPHATEST` flag and the `alphaTest` value passed as a uniform. The declaration of the `alphaTest` uniform is made in the `alphatest_pars_fragment` chunk.
A SpotLight must have its distance set in real world coordinates. This was resulting in very dark scenes, where the position of the light or its intensity must take into account the bounding box. Directional Lights do not require this settings. They light the scene in a uniform manner from afar.
Changes in Three.js regarding light computation even with the `renderer.useLegacyLights` parameter enabled.
A banding effect was due to a loss of precision when copying the texture between render targets.
standard is a linter for js files. Note that it does not work on ts file, which makes the `npm run lint` command inneffective, except for the example files. With the upgrade, standard breaks when it can't find any files, and it reports many warnings for the example files (stylistic warnings). So it is also disabled for now.
ppillot
force-pushed
the
#865-upgrade-three-js-legacy-lights
branch
from
cfc6fe7
to
c23b753
Compare
|
This is a massive amount of work - thank you! I have tried every demo etc and I can't spot regressions between here and master. I did notice some other uncaught regressions that slipped in somewhere earlier - have created a separate issue for those. |
fredludlow
approved these changes
May 3, 2024
Closed
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.
This PR upgrades the Three.js dependency to a more recent version, which still supports the legacy color management. It derives from the #1002 PR which was attempting at migrating the internal color management to a complete linear workflow.
As the current version of Three.js in the codebase raises (unjustified) alerts from npm audit and other dependency alert bots, this intermediary upgrade is somehow necessary.
The
standardlibrary dev dependency has been upgraded as well as 'Jest'.standardis seldom used at the moment, as it does not lint ts files.Known issues
ambientIntensity) and directional lighting (lightIntensity) have been increased to mimic the previous rendering (respectively 0.2 --> 0.3, 1 --> 1.2). This means that users who have custom lighting presets will have to tweak them after the update.standard. This is because the linter does not find any js file in the src directory. I've commented-out the linting commands for now.