Add configurable environment variables #139
Conversation
| constructor( | ||
| scene: Scene, | ||
| camera: PerspectiveCamera, | ||
| spawnSun: boolean = false, |
There was a problem hiding this comment.
I'd eliminate the default value for the spawnSun argument (explicit is always better) otherwise, we'll need to make it the last constructor argument (wouldn't hurt much as we're changing the constructor signature anyway, but as we may want to instantiate the composer with just an empty {} (or just omit completely and have it to be undefined) to be an untouched environmentConfiguration I think it would be good to have it as a last private optional argument).
There was a problem hiding this comment.
Whoops, completely missed that, you're right. I think it makes sense to remove the default argument
| @@ -91,11 +91,17 @@ export class Composer { | |||
| private readonly gaussGrainPass: ShaderPass; | |||
|
|
|||
| private ambientLight: AmbientLight | null = null; | |||
| private environmentConfiguration: EnvironmentConfiguration; | |||
There was a problem hiding this comment.
We can exclude this and receive+declare as a private optional property in the constructor arguments.
| camera: PerspectiveCamera, | ||
| spawnSun: boolean = false, | ||
| environmentConfiguration: EnvironmentConfiguration, | ||
| ) { |
There was a problem hiding this comment.
If the 2 comments above make sense, then we can alter the constructor signature to:
constructor(
scene: Scene,
camera: PerspectiveCamera,
spawnSun: boolean,
private environmentConfiguration?: EnvironmentConfiguration | undefined,
)
| if (environmentConfiguration.postProcessing?.bloomIntensity) { | ||
| extrasValues.bloom = environmentConfiguration.postProcessing.bloomIntensity; | ||
| } | ||
|
|
There was a problem hiding this comment.
if (this.environmentConfiguration?.postProcessing?.bloomIntensity) {
extrasValues.bloom = this.environmentConfiguration.postProcessing.bloomIntensity;
}
| @@ -52,13 +52,32 @@ type MMLDocumentConfiguration = { | |||
| }; | |||
| }; | |||
|
|
|||
| export type EnvironmentConfiguration = { | |||
There was a problem hiding this comment.
We should move this to the core library (in the composer.ts file) minus the enableTweakPane field so that the core doesn't depend on this high level library.
|
|
||
| public sun: Sun | null = null; | ||
| public spawnSun: boolean; | ||
|
|
||
| constructor(scene: Scene, camera: PerspectiveCamera, spawnSun: boolean = false) { | ||
| constructor( |
There was a problem hiding this comment.
nit: make this constructor take an object to disambiguate what the boolean arg is for
| bloomIntensity?: number; | ||
| }, | ||
| } | ||
|
|
||
| export type Networked3dWebExperienceClientConfig = { | ||
| sessionToken: string; |
There was a problem hiding this comment.
I think we can make enableTweakPane part of this config
sachamorgese
force-pushed
the
feature/add-configurable-env-variabels
branch
from
567ac8a to
b02254d
Compare
sachamorgese
changed the title
Increased the reset y value to -100 to allow for underground sections
What kind of change does your PR introduce? (check at least one)