fix(engine): More model fixes (#1797)

visgl · Aug 25, 2023 · 7f2eb66 · 7f2eb66
1 parent e15e572
commit 7f2eb66
Show file tree

Hide file tree

Showing 5 changed files with 41 additions and 79 deletions.
diff --git a/modules/core/src/adapter/device.ts b/modules/core/src/adapter/device.ts
@@ -326,7 +326,7 @@ export abstract class Device {
   protected _getBufferProps(props: BufferProps | ArrayBuffer | ArrayBufferView): BufferProps {
 
     if (props instanceof ArrayBuffer || ArrayBuffer.isView(props)) {
-      return {data: props};
+      props = {data: props};
     }
 
     // TODO - fragile, as this is done before we merge with default options
@@ -339,6 +339,8 @@ export abstract class Device {
         newProps.indexType = 'uint32';
       } else if (props.data instanceof Uint16Array) {
         newProps.indexType = 'uint16';
+      } else {
+        log.warn('indices buffer content must be of integer type')();
       }
     }
     return newProps;    

diff --git a/modules/engine/src/geometry/gpu-geometry.ts b/modules/engine/src/geometry/gpu-geometry.ts
@@ -17,14 +17,7 @@ export type GPUGeometryProps = {
   vertexCount: number;
   bufferLayout: BufferLayout[];
   indices?: Buffer | null;
-  attributes: GPUGeometryAttributes;
-};
-
-export type GPUGeometryAttributes = {
-  positions: Buffer;
-  normals?: Buffer;
-  texCoords?: Buffer;
-  colors?: Buffer;
+  attributes: Record<string, Buffer>;
 };
 
 export class GPUGeometry {
@@ -37,36 +30,17 @@ export class GPUGeometry {
 
   readonly vertexCount: number;
   readonly indices?: Buffer | null;
-  readonly attributes: {
-    positions: Buffer;
-    normals?: Buffer;
-    texCoords?: Buffer;
-    colors?: Buffer;
-  };
+  readonly attributes: Record<string, Buffer>;
 
   constructor(props: GPUGeometryProps) {
     this.id = props.id || uid('geometry');
     this.topology = props.topology;
     this.indices = props.indices || null;
     this.attributes = props.attributes;
 
-    //
     this.vertexCount = props.vertexCount;
 
-    // Populate default bufferLayout
     this.bufferLayout = props.bufferLayout || [];
-    if (!this.bufferLayout.find(layout => layout.name === 'positions')) {
-      this.bufferLayout.push({name: 'positions', format: 'float32x3'});
-    }
-    if (this.attributes.normals && !this.bufferLayout.find(layout => layout.name === 'normals')) {
-      this.bufferLayout.push({name: 'normals', format: 'float32x3'});
-    }
-    if (this.attributes.texCoords && !this.bufferLayout.find(layout => layout.name === 'texCoords')) {
-      this.bufferLayout.push({name: 'texCoords', format: 'float32x2'});
-    }
-    if (this.attributes.colors && !this.bufferLayout.find(layout => layout.name === 'colors')) {
-      this.bufferLayout.push({name: 'colors', format: 'float32x3'});
-    }
 
     if (this.indices) {
       assert(this.indices.usage === Buffer.INDEX);
@@ -85,7 +59,7 @@ export class GPUGeometry {
     return this.vertexCount;
   }
 
-  getAttributes(): GPUGeometryAttributes {
+  getAttributes(): Record<string, Buffer> {
     return this.attributes;
   }
 
@@ -120,36 +94,28 @@ export function getIndexBufferFromGeometry(device: Device, geometry: Geometry):
   if (!geometry.indices) {
     return undefined;
   }
-
   const data = geometry.indices.value;
-  assert(
-    data instanceof Uint16Array || data instanceof Uint32Array,
-    'attribute array for "indices" must be of integer type'
-  );
   return device.createBuffer({usage: Buffer.INDEX, data});
 }
 
 export function getAttributeBuffersFromGeometry(
   device: Device,
   geometry: Geometry
-): {attributes: GPUGeometryAttributes, bufferLayout: BufferLayout[], vertexCount: number} {
-  const positions = geometry.attributes.positions || geometry.attributes.POSITION;
-  const normals = geometry.attributes.normals || geometry.attributes.NORMAL;
-  const texCoords = geometry.attributes.texCoords || geometry.attributes.TEXCOORD_0;
-
-  const attributes: GPUGeometryAttributes = {
-    positions: device.createBuffer({data: positions.value, id: 'positions-buffer'})
-  };
-  const bufferLayout: BufferLayout[] = [
-    {name: 'positions', format: `float32x${positions.size as 2 | 3 | 4}`}
-  ];
-  if (normals) {
-    attributes.normals = device.createBuffer({data: normals.value, id: 'normals-buffer'});
-    bufferLayout.push({name: 'normals', format: `float32x${normals.size as 2 | 3 | 4}`});
-  }
-  if (texCoords) {
-    attributes.texCoords = device.createBuffer({data: texCoords.value, id: 'texCoords-buffer'});
-    bufferLayout.push({name: 'texCoords', format: `float32x${texCoords.size as 2 | 3 | 4}`});
+): {attributes: Record<string, Buffer>, bufferLayout: BufferLayout[], vertexCount: number} {
+  const bufferLayout: BufferLayout[] = [];
+
+  const attributes: Record<string, Buffer> = {};
+  for (const [attributeName, attribute] of Object.entries(geometry.attributes)) {
+    let name: string = attributeName;
+    // TODO Map some GLTF attribute names (is this still needed?)
+    switch (attributeName) {
+      case 'POSITION': name = 'positions'; break;
+      case 'NORMAL': name = 'normals'; break;
+      case 'TEXCOORD_0': name = 'texCoords'; break;
+    }
+
+    attributes[name] = device.createBuffer({data: attribute.value, id: `${attributeName}-buffer`});
+    bufferLayout.push({name: 'normals', format: `float32x${attribute.size as 2 | 3 | 4}`});
   }
 
   const vertexCount = geometry._calculateVertexCount(geometry.attributes, geometry.indices)

diff --git a/modules/engine/src/lib/pipeline-factory.ts b/modules/engine/src/lib/pipeline-factory.ts
@@ -1,3 +1,4 @@
+// luma.gl, MIT license
 import type {RenderPipelineProps} from '@luma.gl/core';
 import {Device, RenderPipeline} from '@luma.gl/core';
 
@@ -87,21 +88,23 @@ export class PipelineFactory {
     const vsHash = this._getHash(props.vs);
     const fsHash = props.fs ? this._getHash(props.fs) : 0;
 
-    // TODO - Can json.stringify() generate different strings for equivalent objects if order of params is different?
-    // create a deepHash() to deduplicate?
-
-    // hash parameters
-    const parameterHash = this._getHash(JSON.stringify(props.parameters));
-
-    // hash buffer layouts
-    const bufferLayoutHash = this._getHash(JSON.stringify(props.bufferLayout));
-
     // WebGL specific
     // const {varyings = [], bufferMode = {}} = props;
     // const varyingHashes = varyings.map((v) => this._getHash(v));
     const varyingHash = '-'; // `${varyingHashes.join('/')}B${bufferMode}`
 
-    return `${vsHash}/${fsHash}V${varyingHash}T${props.topology}P${parameterHash}BL${bufferLayoutHash}}`;
+    switch (this.device.info.type) {
+      case 'webgpu':
+        // On WebGPU we need to rebuild the pipeline if topology, parameters or bufferLayout change
+        const parameterHash = this._getHash(JSON.stringify(props.parameters));
+        const bufferLayoutHash = this._getHash(JSON.stringify(props.bufferLayout));
+        // TODO - Can json.stringify() generate different strings for equivalent objects if order of params is different?
+        // create a deepHash() to deduplicate?
+        return `${vsHash}/${fsHash}V${varyingHash}T${props.topology}P${parameterHash}BL${bufferLayoutHash}}`;
+      default:
+        // WebGL is more dynamic
+        return `${vsHash}/${fsHash}V${varyingHash}`;
+    }
   }
 
   _getHash(key: string): number {

diff --git a/modules/engine/src/model/model.ts b/modules/engine/src/model/model.ts
@@ -225,10 +225,7 @@ export class Model {
   setTopology(topology: PrimitiveTopology): void {
     if (topology !== this.topology) {
       this.topology = topology;
-      // On WebGPU we need to rebuild the pipeline
-      if (this.device.info.type === 'webgpu') {
-        this._setPipelineNeedsUpdate('topology');
-      }
+      this._setPipelineNeedsUpdate('topology');
     }
   }
 
@@ -239,10 +236,7 @@ export class Model {
   setBufferLayout(bufferLayout: BufferLayout[]): void {
     if (bufferLayout !== this.bufferLayout) {
       this.bufferLayout = bufferLayout;
-      // On WebGPU we need to rebuild the pipeline
-      if (this.device.info.type === 'webgpu') {
-        this._setPipelineNeedsUpdate('bufferLayout');
-      }
+      this._setPipelineNeedsUpdate('bufferLayout');
     }
   }
 
@@ -254,10 +248,7 @@ export class Model {
   setParameters(parameters: RenderPipelineParameters) {
     if (!deepEqual(parameters, this.parameters, 2)) {
       this.parameters = parameters;
-      // On WebGPU we need to rebuild the pipeline
-      if (this.device.info.type === 'webgpu') {
-        this._setPipelineNeedsUpdate('parameters');
-      }
+      this._setPipelineNeedsUpdate('parameters');
     }
   }
 

diff --git a/test/render/example-test-cases.ts b/test/render/example-test-cases.ts
@@ -13,11 +13,11 @@ import './disable-example-startup';
 import helloTriangle from '../../examples/getting-started/hello-triangle/app';
 import helloInstancing from '../../examples/getting-started/hello-instancing/app';
 import shaderModules from '../../examples/getting-started/shader-modules/app';
-// import shaderHooks from '../../examples/getting-started/shader-hooks/app';
+import shaderHooks from '../../examples/getting-started/shader-hooks/app';
 // import transformFeedback from '../../examples/getting-started/transform-feedback/app';
 
 // API
-import animation from '../../examples/api/animation/app';
+// import animation from '../../examples/api/animation/app';
 // import texture3d from '../../examples/api/texture-3d/app';
 // import programManagement from '../../examples/api/program-management/app';
 
@@ -35,11 +35,11 @@ const examples = {
   helloTriangle,
   helloInstancing,
   shaderModules,
-  // shaderHooks,
+  shaderHooks,
   // transformFeedback,
 
   // API
-  animation,
+  // animation,
   // texture3d,
   // programManagement,