-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add SpotLight to bitecs #5931
Add SpotLight to bitecs #5931
Conversation
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.
LGTM. Some minor thoughts around typescript stuff but I wouldn't even necessarily change them.
src/inflators/spot-light.ts
Outdated
shadowRadius: number; | ||
}; | ||
|
||
const DEFAULTS = { |
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.
Not sure if I like it better but I notice we have to duplicate a lot of information with these defaults...
We could use typeof: example
src/inflators/spot-light.ts
Outdated
innerConeAngle: number; | ||
outerConeAngle: number; | ||
castShadow: boolean; | ||
shadowMapResolution: [number, number]; |
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.
Not particularly important, but you can name the elements of a "tuple". Gives you slightly nicer type hinting:
shadowMapResolution: [width: number, height: number];
shadowRadius: 1.0 | ||
}; | ||
|
||
export function inflateSpotLight(world: HubsWorld, eid: number, params: SpotLightParams) { |
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.
This was discussed in anotehr PR, so whatever we decide there should apply here too... Since we provide default values the properties passed to the inflator (and exposed to JSX) should be optional, but then the type after applying defaults should have all fields be required. We can do that with typescript Partial/Required utility types.
7ff6301
to
d8cae2b
Compare
This PR adds
SpotLight
to bitecs, similar to #5918.We may want to deprecate light components at some point and encourage to use glTF lights extension, but we don't need to drop the support right now.