Light3d: take a Vector3d for direction and position - #1663
Merged
Conversation
Both were `[number, number, number]` read by index, so a `Vector3d` — the obvious thing to reach for when an option is named `direction` in a 3D engine — set all three components to `undefined`, `normalize()` kept them `NaN`, and the light contributed nothing. Silently: no warning, and the scene still renders with whatever else is lit. `color` on the same options object already accepts a `Color`, a CSS string or an array, so this makes the object consistent with itself rather than adding a new idea. The value is READ, not retained — the light keeps its own normalized vector, and moving yours afterwards does not steer it. Additive: widening the union leaves every array call compiling and on the identical path. The only behaviour that changes is passing a `Vector3d`, which produced no light at all. Three shipped examples got this wrong, which is why it is worth the branch. Closes #1661 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NGvtaUNATVCVxD2qcbiY4t
This file contains hidden or 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
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.
Closes #1661.
directionandpositionwere[number, number, number]read by index, so aVector3d— the obvious thing to reach for when an option is nameddirectionin a 3D engine — set all three components toundefined,normalize()kept themNaN, and the light contributed nothing. Silently: no warning, no throw, and the scene still renders with whatever else is lit.coloron the same options object already accepts aColor, a CSS string or an[r, g, b]array, so this makes the object consistent with itself rather than introducing a new idea. Both options now go through one small reader.The value is read, not retained: the light keeps its own vector, normalized, so moving yours afterwards does not steer it. There is a test for exactly that, because the alternative — aliasing the caller's vector — is the tempting shortcut and would be a different feature.
Backward compatibility
Additive. Widening the union leaves every array call compiling and on the identical path, and the only behaviour that changes is passing a
Vector3d, which previously produced no light at all. Nothing can depend on that.The one caveat from the issue still stands: downstream TypeScript reading
Light3dOptions["direction"]directly and assuming an array would need updating, so this belongs in a minor rather than a patch.Tests
Three, and all three fail without the change — reverting the reader to index-only turns them red with
expected NaN to be -0.356…andexpected [ undefined, undefined, +0 ]:Vector3dand the equivalent array produce byte-identical directionspositionFull suite 282 files / 6867 passed / 9 skipped.
pnpm lintclean.Docs
The class example gains the vector form, the option docs say the value is read rather than retained, and the 3D skill's "this silently yields NaN" trap is gone — it is no longer true. What remains there is the argument-count trap and the Y-down sign, which this does not change.