Code cleanup from review

CesiumGS · Oct 23, 2024 · 4d73f3d · 4d73f3d
1 parent 12588d2
commit 4d73f3d
Show file tree

Hide file tree

Showing 7 changed files with 34 additions and 22 deletions.
diff --git 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'"
           />
-          <input type="text" size="5" data-bind="value: height" />
+          <input type="number" size="5" data-bind="value: height" />
         </div>
       </div>
       <div id="lightingToolbar">
@@ -63,7 +63,7 @@
             step="0.5"
             data-bind="value: directionalLightIntensity, valueUpdate: 'input'"
           />
-          <input type="text" size="5" data-bind="value: directionalLightIntensity" />
+          <input type="number" size="5" data-bind="value: directionalLightIntensity" />
         </div>
         <div class="cesium-button">
           <label>Atmospheric Intensity</label>
@@ -74,7 +74,11 @@
             step="0.5"
             data-bind="value: atmosphereScatteringIntensity, valueUpdate: 'input'"
           />
-          <input type="text" size="5" data-bind="value: atmosphereScatteringIntensity" />
+          <input
+            type="number"
+            size="5"
+            data-bind="value: atmosphereScatteringIntensity"
+          />
         </div>
         <div class="cesium-button">
           <label>Gamma</label>
@@ -85,7 +89,7 @@
             step="0.5"
             data-bind="value: gamma, valueUpdate: 'input'"
           />
-          <input type="text" size="5" data-bind="value: gamma" />
+          <input type="number" size="5" data-bind="value: gamma" />
         </div>
         <div class="cesium-button">
           <label>Brightness</label>
@@ -96,7 +100,7 @@
             step="0.5"
             data-bind="value: brightness, valueUpdate: 'input'"
           />
-          <input type="text" size="5" data-bind="value: brightness" />
+          <input type="number" size="5" data-bind="value: brightness" />
         </div>
         <div class="cesium-button">
           <label>Saturation</label>
@@ -107,7 +111,7 @@
             step="0.5"
             data-bind="value: saturation, valueUpdate: 'input'"
           />
-          <input type="text" size="5" data-bind="value: saturation" />
+          <input type="number" size="5" data-bind="value: saturation" />
         </div>
       </div>
       <div id="environmentToolbar"></div>

diff --git 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<CubeMap.FaceName>}
+ * @type {Iterable<CubeMap.FaceName>}
  * @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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,