From 4d73f3d800452e4e7ab3ed7da114a7a4e686e17d Mon Sep 17 00:00:00 2001 From: ggetz Date: Wed, 23 Oct 2024 09:37:46 -0400 Subject: [PATCH] Code cleanup from review --- .../gallery/development/PBR Lighting.html | 16 ++++++++++------ packages/engine/Source/Renderer/CubeMap.js | 14 +++++++------- .../Source/Scene/DynamicEnvironmentMapManager.js | 4 ++-- .../Model/ImageBasedLightingPipelineStage.js | 8 +++----- .../Functions/computeAtmosphereColor.glsl | 2 +- .../Source/Shaders/Model/GeometryStageFS.glsl | 2 +- packages/engine/Specs/Renderer/CubeMapSpec.js | 10 ++++++++++ 7 files changed, 34 insertions(+), 22 deletions(-) diff --git a/Apps/Sandcastle/gallery/development/PBR Lighting.html b/Apps/Sandcastle/gallery/development/PBR Lighting.html index 33cd0586d034..5b294aa0543b 100644 --- a/Apps/Sandcastle/gallery/development/PBR Lighting.html +++ b/Apps/Sandcastle/gallery/development/PBR Lighting.html @@ -50,7 +50,7 @@ step="200" data-bind="value: height, valueUpdate: 'input'" /> - +
@@ -63,7 +63,7 @@ step="0.5" data-bind="value: directionalLightIntensity, valueUpdate: 'input'" /> - +
@@ -74,7 +74,11 @@ step="0.5" data-bind="value: atmosphereScatteringIntensity, valueUpdate: 'input'" /> - +
@@ -85,7 +89,7 @@ step="0.5" data-bind="value: gamma, valueUpdate: 'input'" /> - +
@@ -96,7 +100,7 @@ step="0.5" data-bind="value: brightness, valueUpdate: 'input'" /> - +
@@ -107,7 +111,7 @@ step="0.5" data-bind="value: saturation, valueUpdate: 'input'" /> - +
diff --git a/packages/engine/Source/Renderer/CubeMap.js b/packages/engine/Source/Renderer/CubeMap.js index c31db225cc25..7526ed2b42a4 100644 --- a/packages/engine/Source/Renderer/CubeMap.js +++ b/packages/engine/Source/Renderer/CubeMap.js @@ -112,7 +112,7 @@ function CubeMap(options) { ({ width, height } = source.positiveX); //>>includeStart('debug', pragmas.debug); - for (const faceName of CubeMap.faces()) { + for (const faceName of CubeMap.faceNames()) { const face = source[faceName]; if (Number(face.width) !== width || Number(face.height) !== height) { throw new DeveloperError( @@ -238,7 +238,7 @@ function CubeMap(options) { ); } - for (const faceName of CubeMap.faces()) { + for (const faceName of CubeMap.faceNames()) { loadFace(this[faceName], source?.[faceName], 0); } @@ -288,7 +288,7 @@ CubeMap.FaceName = Object.freeze({ NEGATIVEZ: "negativeZ", }); -function* makeFacesIterator() { +function* makeFaceNamesIterator() { yield CubeMap.FaceName.POSITIVEX; yield CubeMap.FaceName.NEGATIVEX; yield CubeMap.FaceName.POSITIVEY; @@ -299,11 +299,11 @@ function* makeFacesIterator() { /** * Creates an iterator for looping over the cubemap faces. - * @type {iterable} + * @type {Iterable} * @private */ -CubeMap.faces = function () { - return makeFacesIterator(); +CubeMap.faceNames = function () { + return makeFaceNamesIterator(); }; /** @@ -593,7 +593,7 @@ CubeMap.prototype.loadMipmaps = function (source, skipColorSpaceConversion) { const mipSource = source[i]; // mipLevel 0 was the base layer, already loaded when the CubeMap was constructed. const mipLevel = i + 1; - for (const faceName of CubeMap.faces()) { + for (const faceName of CubeMap.faceNames()) { loadFace(this[faceName], mipSource[faceName], mipLevel); } } diff --git a/packages/engine/Source/Scene/DynamicEnvironmentMapManager.js b/packages/engine/Source/Scene/DynamicEnvironmentMapManager.js index bccfd222452d..3fd314bdbc1d 100644 --- a/packages/engine/Source/Scene/DynamicEnvironmentMapManager.js +++ b/packages/engine/Source/Scene/DynamicEnvironmentMapManager.js @@ -463,7 +463,7 @@ function updateRadianceMap(manager, frameState) { } let i = 0; - for (const face of CubeMap.faces()) { + for (const face of CubeMap.faceNames()) { let texture = manager._radianceMapTextures[i]; if (defined(texture)) { texture.destroy(); @@ -569,7 +569,7 @@ function updateSpecularMaps(manager, frameState) { let index = 0; for (let level = 1; level < mipmapLevels; ++level) { - for (const face of CubeMap.faces()) { + for (const face of CubeMap.faceNames()) { const texture = (manager._specularMapTextures[index] = new Texture({ context: context, width: width, diff --git a/packages/engine/Source/Scene/Model/ImageBasedLightingPipelineStage.js b/packages/engine/Source/Scene/Model/ImageBasedLightingPipelineStage.js index 4ed8a4e83227..43e4b52ddd32 100644 --- a/packages/engine/Source/Scene/Model/ImageBasedLightingPipelineStage.js +++ b/packages/engine/Source/Scene/Model/ImageBasedLightingPipelineStage.js @@ -1,5 +1,4 @@ import combine from "../../Core/combine.js"; -import defaultValue from "../../Core/defaultValue.js"; import defined from "../../Core/defined.js"; import ImageBasedLightingStageFS from "../../Shaders/Model/ImageBasedLightingStageFS.js"; import ShaderDestination from "../../Renderer/ShaderDestination.js"; @@ -33,10 +32,9 @@ ImageBasedLightingPipelineStage.process = function ( if (!defined(imageBasedLighting.specularEnvironmentMaps)) { specularEnvironmentMapAtlas = environmentMapManager.radianceCubeMap; } - const sphericalHarmonicCoefficients = defaultValue( - imageBasedLighting.sphericalHarmonicCoefficients, - environmentMapManager.sphericalHarmonicCoefficients, - ); + const sphericalHarmonicCoefficients = + imageBasedLighting.sphericalHarmonicCoefficients ?? + environmentMapManager.sphericalHarmonicCoefficients; shaderBuilder.addDefine( "USE_IBL_LIGHTING", diff --git a/packages/engine/Source/Shaders/Builtin/Functions/computeAtmosphereColor.glsl b/packages/engine/Source/Shaders/Builtin/Functions/computeAtmosphereColor.glsl index 4a15bf21072c..e4697721b7fd 100644 --- a/packages/engine/Source/Shaders/Builtin/Functions/computeAtmosphereColor.glsl +++ b/packages/engine/Source/Shaders/Builtin/Functions/computeAtmosphereColor.glsl @@ -51,7 +51,7 @@ vec4 czm_computeAtmosphereColor( * @name czm_computeAtmosphereColor * @glslFunction * - * @param {czm_rat} primaryRay Ray from the origin to sky fragment to in world coords (low precision) + * @param {czm_ray} primaryRay Ray from the origin to sky fragment to in world coords (low precision) * @param {vec3} lightDirection Light direction from the sun or other light source. * @param {vec3} rayleighColor The Rayleigh scattering color computed by a scattering function * @param {vec3} mieColor The Mie scattering color computed by a scattering function diff --git a/packages/engine/Source/Shaders/Model/GeometryStageFS.glsl b/packages/engine/Source/Shaders/Model/GeometryStageFS.glsl index 5912f1528afe..6f5217a11ba3 100644 --- a/packages/engine/Source/Shaders/Model/GeometryStageFS.glsl +++ b/packages/engine/Source/Shaders/Model/GeometryStageFS.glsl @@ -3,7 +3,7 @@ void geometryStage(out ProcessedAttributes attributes) attributes.positionMC = v_positionMC; attributes.positionEC = v_positionEC; - #if defined(COMPUTE_POSITION_WC_CUSTOM_SHADER) || defined(COMPUTE_POSITION_WC_STYLE) || defined(COMPUTE_POSITION_WC_ATMOSPHERE) + #if defined(COMPUTE_POSITION_WC_CUSTOM_SHADER) || defined(COMPUTE_POSITION_WC_STYLE) || defined(COMPUTE_POSITION_WC_ATMOSPHERE) attributes.positionWC = v_positionWC; #endif diff --git a/packages/engine/Specs/Renderer/CubeMapSpec.js b/packages/engine/Specs/Renderer/CubeMapSpec.js index 86a97c7c28a8..2167bcc9f20a 100644 --- a/packages/engine/Specs/Renderer/CubeMapSpec.js +++ b/packages/engine/Specs/Renderer/CubeMapSpec.js @@ -246,6 +246,16 @@ describe( expect(cubeMap.flipY).toEqual(true); }); + it("faceNames returns an iterator over each of the faces by name", () => { + let count = 0; + for (const faceName of CubeMap.faceNames()) { + expect(Object.values(CubeMap.FaceName).includes(faceName)).toBeTrue(); + count++; + } + + expect(count).toBe(6); + }); + it("draws with a cube map", function () { cubeMap = new CubeMap({ context: webgl2Context,