Minor code cleanup (#2459)

* removed offscreen canvas vestage

* cleanup conversions

* added missing semicolons

* added missing <void>s

* clean up RE

* deflake space-opera tests

packages/model-viewer/src/constants.ts

@@ -44,13 +44,6 @@ export const IS_MOBILE = (() => {
 
 export const IS_CHROMEOS = /\bCrOS\b/.test(navigator.userAgent);
 
-// Disabling offscreen canvas for now because it is slower and has bugs relating
-// to janky updates and out of sync frames.
-export const USE_OFFSCREEN_CANVAS = false;
-// Boolean((self as any).OffscreenCanvas) &&
-//     Boolean((self as any).OffscreenCanvas.prototype.transferToImageBitmap) &&
-//     !IS_CHROMEOS;  // TODO(elalish): file a bug on inverted renders
-
 export const IS_ANDROID = /android/i.test(navigator.userAgent);
 
 // Prior to iOS 13, detecting iOS Safari was relatively straight-forward.

packages/model-viewer/src/model-viewer-base.ts

@@ -454,7 +454,7 @@ export default class ModelViewerElementBase extends UpdatingElement {
 
           resolve(blob);
         }, mimeType, qualityArgument);
-      })
+      });
     } finally {
       this[$updateSize]({width, height});
     };

packages/model-viewer/src/styles/conversions.ts

@@ -125,13 +125,13 @@ export const normalizeUnit = (() => {
   };
 
   return (node: NumberNode, fallback: NumberNode = ZERO) => {
-    let {number, unit} = node;
-
-    if (!isFinite(number)) {
-      number = fallback.number;
-      unit = fallback.unit;
+    if (!isFinite(node.number)) {
+      node.number = fallback.number;
+      node.unit = fallback.unit;
     }
 
+    const {unit} = node;
+
     if (unit == null) {
       return node;
     }

packages/model-viewer/src/styles/parsers.ts

@@ -203,7 +203,7 @@ const parseExpression = (() => {
  * An ident is something like a function name or the keyword "auto".
  */
 const parseIdent = (() => {
-  const NOT_IDENT_RE = /[^a-z^0-9^_^\-^\u0240-\uffff]/i;
+  const NOT_IDENT_RE = /[^a-z0-9_\-\u0240-\uffff]/i;
 
   return (inputString: string): ParseResult<IdentNode> => {
     const match = inputString.match(NOT_IDENT_RE);

packages/model-viewer/src/test/three-components/Renderer-spec.ts

@@ -13,7 +13,6 @@
  * limitations under the License.
  */
 
-import {USE_OFFSCREEN_CANVAS} from '../../constants.js';
 import {$controls} from '../../features/controls.js';
 import {$intersectionObserver, $isElementInViewport, $onResize, $renderer, $scene, Camera, RendererInterface} from '../../model-viewer-base.js';
 import {ModelViewerElement} from '../../model-viewer.js';
@@ -189,7 +188,7 @@ suite('Renderer with two scenes', () => {
     test('uses the proper canvas when unregsitering scenes', () => {
       renderer.render(performance.now());
 
-      expect(renderer.canvasElement.classList.contains('show'))
+      expect(renderer.canvas3D.classList.contains('show'))
           .to.be.eq(
               false, 'webgl canvas should not be shown with multiple scenes.');
       expect(scene.canvas.classList.contains('show'))
@@ -201,20 +200,14 @@ suite('Renderer with two scenes', () => {
       renderer.unregisterScene(scene);
       renderer.render(performance.now());
 
-      if (USE_OFFSCREEN_CANVAS) {
-        expect(renderer.canvasElement.classList.contains('show'))
-            .to.be.eq(false);
-        expect(otherScene.canvas.classList.contains('show')).to.be.eq(true);
-      } else {
-        expect(renderer.canvasElement.parentElement)
-            .to.be.eq(otherScene.canvas.parentElement);
-        expect(renderer.canvasElement.classList.contains('show'))
-            .to.be.eq(true, 'webgl canvas should be shown with single scene.');
-        expect(otherScene.canvas.classList.contains('show'))
-            .to.be.eq(
-                false,
-                'otherScene canvas should not be shown when it is the only scene.');
-      }
+      expect(renderer.canvas3D.parentElement)
+          .to.be.eq(otherScene.canvas.parentElement);
+      expect(renderer.canvas3D.classList.contains('show'))
+          .to.be.eq(true, 'webgl canvas should be shown with single scene.');
+      expect(otherScene.canvas.classList.contains('show'))
+          .to.be.eq(
+              false,
+              'otherScene canvas should not be shown when it is the only scene.');
     });
 
     suite('when resizing', () => {

packages/model-viewer/src/three-components/ARRenderer.ts

@@ -66,7 +66,7 @@ export type ARTracking = 'tracking'|'not-tracking';
 export const ARTracking: {[index: string]: ARTracking} = {
   TRACKING: 'tracking',
   NOT_TRACKING: 'not-tracking'
-}
+};
 
 export interface ARTrackingEvent extends ThreeEvent {
   status: ARTracking,

packages/model-viewer/src/three-components/ModelScene.ts

@@ -16,7 +16,6 @@
 import {AnimationAction, AnimationClip, AnimationMixer, Box3, Event as ThreeEvent, Matrix3, Object3D, PerspectiveCamera, Raycaster, Scene, Vector2, Vector3} from 'three';
 import {CSS2DRenderer} from 'three/examples/jsm/renderers/CSS2DRenderer';
 
-import {USE_OFFSCREEN_CANVAS} from '../constants.js';
 import ModelViewerElementBase, {$renderer, RendererInterface} from '../model-viewer-base.js';
 import {resolveDpr} from '../utilities.js';
 
@@ -148,11 +147,7 @@ export class ModelScene extends Scene {
    * there are more than one.
    */
   createContext() {
-    if (USE_OFFSCREEN_CANVAS) {
-      this.context = this.canvas.getContext('bitmaprenderer')!;
-    } else {
-      this.context = this.canvas.getContext('2d')!;
-    }
+    this.context = this.canvas.getContext('2d')!;
   }
 
   /**

packages/model-viewer/src/three-components/Renderer.ts

@@ -16,7 +16,6 @@
 import {ACESFilmicToneMapping, Event, EventDispatcher, GammaEncoding, PCFSoftShadowMap, WebGLRenderer} from 'three';
 import {RoughnessMipmapper} from 'three/examples/jsm/utils/RoughnessMipmapper';
 
-import {USE_OFFSCREEN_CANVAS} from '../constants.js';
 import {$canvas, $tick, $updateSize} from '../model-viewer-base.js';
 import {clamp, isDebugMode, resolveDpr} from '../utilities.js';
 
@@ -68,8 +67,7 @@ export class Renderer extends EventDispatcher {
   }
 
   public threeRenderer!: WebGLRenderer;
-  public canvasElement: HTMLCanvasElement;
-  public canvas3D: HTMLCanvasElement|OffscreenCanvas;
+  public canvas3D: HTMLCanvasElement;
   public textureUtils: TextureUtils|null;
   public arRenderer: ARRenderer;
   public roughnessMipmapper: RoughnessMipmapper;
@@ -111,13 +109,8 @@ export class Renderer extends EventDispatcher {
 
     this.dpr = resolveDpr();
 
-    this.canvasElement = document.createElement('canvas');
-    this.canvasElement.id = 'webgl-canvas';
-
-    this.canvas3D = USE_OFFSCREEN_CANVAS ?
-        this.canvasElement.transferControlToOffscreen() :
-        this.canvasElement;
-
+    this.canvas3D = document.createElement('canvas');
+    this.canvas3D.id = 'webgl-canvas';
     this.canvas3D.addEventListener('webglcontextlost', this.onWebGLContextLost);
 
     try {
@@ -199,8 +192,8 @@ export class Renderer extends EventDispatcher {
     const heightCSS = height / scale;
     // The canvas element must by styled outside of three due to the offscreen
     // canvas not being directly stylable.
-    this.canvasElement.style.width = `${widthCSS}px`;
-    this.canvasElement.style.height = `${heightCSS}px`;
+    this.canvas3D.style.width = `${widthCSS}px`;
+    this.canvas3D.style.height = `${heightCSS}px`;
 
     // Each scene's canvas must match the renderer size. In general they can be
     // larger than the element that contains them, but the overflow is hidden
@@ -235,8 +228,8 @@ export class Renderer extends EventDispatcher {
     const width = this.width / scale;
     const height = this.height / scale;
 
-    this.canvasElement.style.width = `${width}px`;
-    this.canvasElement.style.height = `${height}px`;
+    this.canvas3D.style.width = `${width}px`;
+    this.canvas3D.style.height = `${height}px`;
     for (const scene of this.scenes) {
       const {style} = scene.canvas;
       style.width = `${width}px`;
@@ -284,8 +277,7 @@ export class Renderer extends EventDispatcher {
   }
 
   displayCanvas(scene: ModelScene): HTMLCanvasElement {
-    return this.multipleScenesVisible ? scene.element[$canvas] :
-                                        this.canvasElement;
+    return this.multipleScenesVisible ? scene.element[$canvas] : this.canvas3D;
   }
 
   /**
@@ -307,18 +299,18 @@ export class Renderer extends EventDispatcher {
     if (visibleCanvas == null) {
       return;
     }
-    const multipleScenesVisible = visibleScenes > 1 || USE_OFFSCREEN_CANVAS;
-    const {canvasElement} = this;
+    const multipleScenesVisible = visibleScenes > 1;
+    const {canvas3D} = this;
 
     if (multipleScenesVisible === this.multipleScenesVisible &&
         (multipleScenesVisible ||
-         canvasElement.parentElement === visibleCanvas.parentElement)) {
+         canvas3D.parentElement === visibleCanvas.parentElement)) {
       return;
     }
     this.multipleScenesVisible = multipleScenesVisible;
 
     if (multipleScenesVisible) {
-      canvasElement.classList.remove('show');
+      canvas3D.classList.remove('show');
     }
     for (const scene of this.scenes) {
       if (scene.externalRenderer != null) {
@@ -329,8 +321,8 @@ export class Renderer extends EventDispatcher {
         canvas.classList.add('show');
         scene.isDirty = true;
       } else if (scene.canvas === visibleCanvas) {
-        scene.canvas.parentElement!.appendChild(canvasElement);
-        canvasElement.classList.add('show');
+        scene.canvas.parentElement!.appendChild(canvas3D);
+        canvas3D.classList.add('show');
         canvas.classList.remove('show');
         scene.isDirty = true;
       }
@@ -456,17 +448,10 @@ export class Renderer extends EventDispatcher {
         if (scene.context == null) {
           scene.createContext();
         }
-        if (USE_OFFSCREEN_CANVAS) {
-          const contextBitmap = scene.context as ImageBitmapRenderingContext;
-          const bitmap =
-              (this.canvas3D as OffscreenCanvas).transferToImageBitmap();
-          contextBitmap.transferFromImageBitmap(bitmap);
-        } else {
-          const context2D = scene.context as CanvasRenderingContext2D;
-          context2D.clearRect(0, 0, width, height);
-          context2D.drawImage(
-              this.canvas3D, 0, 0, width, height, 0, 0, width, height);
-        }
+        const context2D = scene.context as CanvasRenderingContext2D;
+        context2D.clearRect(0, 0, width, height);
+        context2D.drawImage(
+            this.canvas3D, 0, 0, width, height, 0, 0, width, height);
       }
 
       scene.isDirty = false;

packages/model-viewer/src/utilities.ts

@@ -14,6 +14,7 @@
  */
 
 import {EventDispatcher} from 'three';
+
 import {HAS_WEBXR_DEVICE_API, HAS_WEBXR_HIT_TEST_API, IS_WEBXR_AR_CANDIDATE} from './constants.js';
 
 export type Constructor<T = object, U = object> = {
@@ -189,8 +190,7 @@ export const resolveDpr: () => number = (() => {
  */
 export const isDebugMode = (() => {
   const debugQueryParameterName = 'model-viewer-debug-mode';
-  const debugQueryParameter =
-      new RegExp(`[\?&]${debugQueryParameterName}(&|$)`);
+  const debugQueryParameter = new RegExp(`[?&]${debugQueryParameterName}(&|$)`);
 
   return () => ((self as any).ModelViewerElement &&
                 (self as any).ModelViewerElement.debugMode) ||

packages/render-fidelity-tools/src/artifact-creator.ts

@@ -158,7 +158,7 @@ export class ArtifactCreator {
     if (rmsInDb > FIDELITY_TEST_THRESHOLD) {
       if (exclude?.includes('model-viewer')) {
         console.log(`❌ Skipped! Senario name: ${
-            scenario.name}, rms distance ratio: ${rmsInDb.toFixed(2)} dB.`)
+            scenario.name}, rms distance ratio: ${rmsInDb.toFixed(2)} dB.`);
       } else {
         throw new Error(`❌ Senarios name: ${
             scenario.name}, rms distance ratio: ${rmsInDb.toFixed(2)} dB.`);
@@ -305,7 +305,7 @@ export class ArtifactCreator {
     // currently has no mechanism to detect this and will happily tell you
     // your code is correct when it isn't.
     const evaluateError = await page.evaluate(async (maxTimeInSec) => {
-      const modelBecomesReady = new Promise((resolve, reject) => {
+      const modelBecomesReady = new Promise<void>((resolve, reject) => {
         let timeout: NodeJS.Timeout;
         if (maxTimeInSec > 0) {
           timeout = setTimeout(() => {

packages/render-fidelity-tools/src/common.ts

@@ -221,10 +221,8 @@ export class ImageComparator {
     }
 
     return {
-      imageBuffers: {
-        delta: deltaImage ? deltaImage.buffer : null,
-        blackWhite: blackWhiteImage ? blackWhiteImage.buffer : null
-      }
+      imageBuffers:
+          {delta: deltaImage.buffer, blackWhite: blackWhiteImage.buffer}
     };
   }
 

packages/render-fidelity-tools/src/components/renderer-harness.ts

@@ -58,7 +58,7 @@ export class RendererConfiguration extends LitElement {
       if (this.configUrl == null) {
         this.config = null;
       } else {
-        this.config = await (await fetch(this.configUrl)).json()
+        this.config = await (await fetch(this.configUrl)).json();
       }
     }
 

packages/render-fidelity-tools/src/components/renderers/filament-viewer.ts

@@ -218,7 +218,7 @@ export class FilamentViewer extends LitElement {
       await fetchFilamentAssets([iblUrl, skyboxUrl]);
       const ibl = this[$engine].createIblFromKtx(iblUrl);
       this[$scene].setIndirectLight(ibl);
-      this[$ibl] = ibl
+      this[$ibl] = ibl;
       ibl.setIntensity(1.0);
       ibl.setRotation([0, 0, -1, 0, 1, 0, 1, 0, 0]);  // 90 degrees
 
@@ -243,7 +243,7 @@ export class FilamentViewer extends LitElement {
 
     const asset = this[$currentAsset]!;
 
-    await new Promise((resolve) => {
+    await new Promise<void>((resolve) => {
       console.log('Loading resources for', modelUrl);
       asset.loadResources(resolve, () => {}, basepath(modelUrl), 1);
     });

packages/render-fidelity-tools/src/workflows/update-screenshots.ts

@@ -20,6 +20,7 @@ import {dirname, join, resolve} from 'path';
 
 import {ImageComparisonConfig} from '../common.js';
 import {ConfigReader} from '../config-reader.js';
+
 import {rendererScreenshot} from './update-screenshots/renderer-screenshot.js';
 
 const require = module.createRequire(import.meta.url);
@@ -36,7 +37,7 @@ const config = require(configPath);
 
 const {renderers} = config;
 const goldensDirectory = join(rootDirectory, 'goldens');
-const renderersDirectory = join(rootDirectory, 'renderers')
+const renderersDirectory = join(rootDirectory, 'renderers');
 
 let scenarioWhitelist: Set<string>|null = null;
 let rendererWhitelist: Set<string>|null = null;
@@ -68,25 +69,26 @@ const run = async (
     command: string,
     args: Array<string>,
     environmentVariables = {},
-    workingDirectory = process.cwd()) => new Promise((resolve, reject) => {
-  const childProcess = spawn(command, args, {
-    cwd: workingDirectory,
-    env: {...process.env, ...environmentVariables},
-    stdio: ['ignore', 'inherit', 'inherit']
-  });
-
-  childProcess.once('error', (error: any) => {
-    warn(error);
-  });
-
-  childProcess.once('exit', (code) => {
-    if (code === 0) {
-      resolve();
-    } else {
-      reject(new Error('Command failed'));
-    }
-  });
-});
+    workingDirectory = process.cwd()) =>
+    new Promise<void>((resolve, reject) => {
+      const childProcess = spawn(command, args, {
+        cwd: workingDirectory,
+        env: {...process.env, ...environmentVariables},
+        stdio: ['ignore', 'inherit', 'inherit']
+      });
+
+      childProcess.once('error', (error: any) => {
+        warn(error);
+      });
+
+      childProcess.once('exit', (code) => {
+        if (code === 0) {
+          resolve();
+        } else {
+          reject(new Error('Command failed'));
+        }
+      });
+    });
 
 const updateScreenshots = async (config: ImageComparisonConfig) => {
   const {scenarios} = config;

packages/space-opera/src/components/config/reducer.ts

@@ -86,7 +86,7 @@ export function dispatchSetReveal(reveal?: string) {
   return {type: SET_REVEAL, payload: reveal};
 }
 
-const SET_CONFIG = 'SET_CONFIG'
+const SET_CONFIG = 'SET_CONFIG';
 export function dispatchSetConfig(config: ModelViewerConfig) {
   return {type: SET_CONFIG, payload: config};
 }