PointLight/SpotLight: Deprecate distance parameter, auto-compute from intensity and decay. #32543
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
|
Improved Screen.Recording.2025-12-13.at.17.16.29.mov |
mrdoob
force-pushed
the
light-distance
branch
from
85e73b4 to
646e7f0
Compare
|
Seems like |
|
Note that this will mean dropping support for the
That said, I don't know if it's applied in this way by three.js users. If we prefer to not support this field in the glTF extension spec, that's OK with me. |
Changes SummaryThis PR removes the redundant Type: bugfix Components Affected: PointLight, SpotLight, PointLightHelper, ObjectLoader, Editor, Examples (webgl and webgpu), Manual documentation Files Changed
Architecture Impact
Risk Areas: Backward compatibility: Scenes saved with explicit distance values will lose that information when loaded, API contract violation: distance property setter exists but doesn't persist the value (silent behavior change), Physics correctness assumption: The formula Math.pow(intensity/0.01, 1/decay) assumes a specific light falloff model that may not apply universally, Editor compatibility: Editor changes suggest UI updates may not be complete across all use cases, Example parameter adjustments: intensity values doubled in some examples (200→400) which alters visual appearance, FBXLoader and other loaders: Changes to light instantiation may affect import behavior from external formats Suggestions
Full review in progress... | Powered by diffray |
| if ( camera.isPerspectiveCamera ) { | ||
|
|
||
| const fov = camera.fov * ( Math.PI / 180 ); | ||
| const scale = distance * Math.tan( fov / 2 ) * this.size * 0.04; |
There was a problem hiding this comment.
🟡 MEDIUM - Magic number 0.04 without explanation
Agent: quality
Category: quality
Description:
The constant 0.04 is used in a scale calculation without explanation. This appears to be a tuning parameter for screen-space size calculation.
Suggestion:
Extract to a named constant at module level with a descriptive name like 'SCREEN_SPACE_SCALE_FACTOR' and document its purpose.
Confidence: 70%
Rule: qual_magic_numbers_js
Review ID: d45dfd15-2438-493b-ae82-0d50672f888b
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| 'bottom': objectBottomRow, | ||
| 'near': objectNearRow, | ||
| 'far': objectFarRow, | ||
| 'intensity': objectIntensityRow, |
There was a problem hiding this comment.
🔵 LOW - console.warn used for exception logging
Agent: quality
Category: quality
Description:
Production code uses console.warn to log exceptions. Should use structured logging instead.
Suggestion:
Replace console.warn with a project-level logging utility or consider if this logging is needed.
Confidence: 60%
Rule: quality_avoid_console_in_production
Review ID: d45dfd15-2438-493b-ae82-0d50672f888b
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
|
|
||
| /** | ||
| * Constructs a new spot light. | ||
| * |
There was a problem hiding this comment.
🔵 LOW - Typo in JSDoc code example
Agent: react
Category: docs
Description:
JSDoc code example contains extra 's' character: 'spotLight.shadow.camera.fov = 30;s' should be 'spotLight.shadow.camera.fov = 30;'
Suggestion:
Remove the trailing 's' character from line 22.
Confidence: 100%
Rule: ts_always_use_jsdoc_for_documentation
Review ID: d45dfd15-2438-493b-ae82-0d50672f888b
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| import { PointLightHelper } from '../../../../src/helpers/PointLightHelper.js'; | ||
|
|
||
| import { Mesh } from '../../../../src/objects/Mesh.js'; | ||
| import { PointLight } from '../../../../src/lights/PointLight.js'; | ||
|
|
||
| export default QUnit.module( 'Helpers', () => { | ||
|
|
||
| QUnit.module( 'PointLightHelper', () => { | ||
|
|
||
| const parameters = { | ||
| sphereSize: 1, | ||
| size: 1, | ||
| color: 0xaaaaaa, | ||
| intensity: 0.5, | ||
| distance: 100, | ||
| decay: 2 | ||
| }; | ||
|
|
||
| // INHERITANCE | ||
| QUnit.test( 'Extending', ( assert ) => { | ||
|
|
||
| const light = new PointLight( parameters.color ); | ||
| const object = new PointLightHelper( light, parameters.sphereSize, parameters.color ); | ||
| assert.strictEqual( | ||
| object instanceof Mesh, true, | ||
| 'PointLightHelper extends from Mesh' | ||
| ); | ||
|
|
||
| } ); | ||
|
|
||
| // INSTANCING | ||
| QUnit.test( 'Instancing', ( assert ) => { | ||
|
|
||
| const light = new PointLight( parameters.color ); | ||
| const object = new PointLightHelper( light, parameters.sphereSize, parameters.color ); | ||
| const object = new PointLightHelper( light, parameters.size ); | ||
| assert.ok( object, 'Can instantiate a PointLightHelper.' ); | ||
|
|
||
| } ); | ||
|
|
||
| // PROPERTIES | ||
| QUnit.test( 'type', ( assert ) => { | ||
|
|
||
| const light = new PointLight( parameters.color ); | ||
| const object = new PointLightHelper( light, parameters.sphereSize, parameters.color ); | ||
| const object = new PointLightHelper( light, parameters.size ); | ||
| assert.ok( | ||
| object.type === 'PointLightHelper', | ||
| 'PointLightHelper.type should be PointLightHelper' | ||
| ); | ||
|
|
||
| } ); | ||
|
|
||
| // PUBLIC | ||
| QUnit.test( 'dispose', ( assert ) => { | ||
|
|
||
| assert.expect( 0 ); | ||
|
|
||
| const light = new PointLight( parameters.color ); | ||
| const object = new PointLightHelper( light, parameters.sphereSize, parameters.color ); | ||
| const object = new PointLightHelper( light, parameters.size ); | ||
| object.dispose(); | ||
|
|
||
| } ); |
There was a problem hiding this comment.
🟠 HIGH - New PointLightHelper 'size' parameter lacks test coverage
Agent: testing
Category: quality
Description:
PointLightHelper constructor has size parameter (default=1) but tests don't verify the property is set correctly or test default behavior.
Suggestion:
Add tests: verify helper.size equals passed value, test default value when size omitted, test with different values (0, 2, etc.)
Confidence: 80%
Rule: test_new_parameter_coverage
Review ID: d45dfd15-2438-493b-ae82-0d50672f888b
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| let lights = undefined; | ||
| hooks.beforeEach( function () { | ||
|
|
||
| const parameters = { |
There was a problem hiding this comment.
🟠 HIGH - PointLight distance getter lacks test coverage
Agent: testing
Category: quality
Description:
PointLight.distance is now a computed getter (lines 94-106) based on intensity/decay. Tests don't verify this behavior.
Suggestion:
Add test: create light with known intensity/decay, assert distance getter returns expected computed value. Also test distance setter shows deprecation warning.
Confidence: 85%
Rule: test_new_parameter_coverage
Review ID: d45dfd15-2438-493b-ae82-0d50672f888b
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| let lights = undefined; | ||
| hooks.beforeEach( function () { | ||
|
|
||
| const parameters = { |
There was a problem hiding this comment.
🟠 HIGH - SpotLight distance getter lacks test coverage
Agent: testing
Category: quality
Description:
SpotLight.distance is now a computed getter (lines 151-163) based on intensity/decay. Tests don't verify this behavior.
Suggestion:
Add test: create light with known intensity/decay, assert distance getter returns expected computed value. Also test distance setter shows deprecation warning.
Confidence: 85%
Rule: test_new_parameter_coverage
Review ID: d45dfd15-2438-493b-ae82-0d50672f888b
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
Review Summary
Validated 42 issues: 8 kept, 34 filtered Issues Found: 8💬 See 6 individual line comment(s) for details. 📊 5 unique issue type(s) across 8 location(s) 📋 Full issue list (click to expand)🟠 HIGH - New PointLightHelper 'size' parameter lacks test coverage (3 occurrences)Agent: testing Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Magic number 0.04 without explanationAgent: quality Category: quality File: Description: The constant 0.04 is used in a scale calculation without explanation. This appears to be a tuning parameter for screen-space size calculation. Suggestion: Extract to a named constant at module level with a descriptive name like 'SCREEN_SPACE_SCALE_FACTOR' and document its purpose. Confidence: 70% Rule: 🟡 MEDIUM - Large block of commented-out codeAgent: refactoring Category: quality File: Description: 40-line commented code block with implementation logic (object actions UI with switch statements). Should either be removed or restored. Suggestion: Remove the entire commented block (lines 29-68) unless it will be restored soon. Use version control for code history instead. Confidence: 85% Rule: 🔵 LOW - console.warn used for exception logging (2 occurrences)Agent: quality Category: quality 📍 View all locations
Rule: 🔵 LOW - Typo in JSDoc code exampleAgent: react Category: docs File: Description: JSDoc code example contains extra 's' character: 'spotLight.shadow.camera.fov = 30;s' should be 'spotLight.shadow.camera.fov = 30;' Suggestion: Remove the trailing 's' character from line 22. Confidence: 100% Rule: ℹ️ 2 issue(s) outside PR diff (click to expand)
🟡 MEDIUM - Large block of commented-out codeAgent: refactoring Category: quality File: Description: 40-line commented code block with implementation logic (object actions UI with switch statements). Should either be removed or restored. Suggestion: Remove the entire commented block (lines 29-68) unless it will be restored soon. Use version control for code history instead. Confidence: 85% Rule: 🔵 LOW - console.log used for error loggingAgent: quality Category: quality File: Description: Production code uses console.log to log JSON parsing errors. Should use console.error or structured logging. Suggestion: Replace console.log with console.error or a project-level logging utility. Confidence: 62% Rule: Review ID: |
Description
The
distanceparameter was redundant - it can be derived from the physically-basedintensityanddecayvalues. This simplifies the API and ensures consistency between visual range and actual light falloff.