From f913222e1b08a8892ec187c34094329236c565cb Mon Sep 17 00:00:00 2001 From: Erik Onarheim Date: Wed, 28 Feb 2024 20:44:01 -0600 Subject: [PATCH] fix: Firefox + Linux custom shader issue with glsl compiler attribute optimization (#2943) This PR fixes an issue where Firefox on Linux would throw an error when using custom Materials due to unused attributes caused by glsl compiler optimization. Before it would throw and crash excalibur ![image](https://github.com/excaliburjs/Excalibur/assets/612071/6b6a3f55-d260-4663-92d7-39ac8e3aa497) Now after, it will log a warning because this is potentially a bug, or at minimum something you should look at ![image](https://github.com/excaliburjs/Excalibur/assets/612071/4eb162c2-b559-4923-81e7-8723b758de36) --- CHANGELOG.md | 1 + src/engine/Graphics/Context/material.ts | 1 + src/engine/Graphics/Context/vertex-layout.ts | 50 +++++++++++++---- src/engine/Graphics/Context/webgl-util.ts | 56 ++++++++++++++++++++ src/spec/VertexLayoutSpec.ts | 2 +- 5 files changed, 100 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d59c2d398..1f746d133 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). ### Fixed +- Fixed issue where Firefox on Linux would throw an error when using custom Materials due to unused attributes caused by glsl compiler optimization. - Fixed issue where start transition did not work properly if deferred - Fixed issue where transitions did not cover the whole screen if camera was zoomed diff --git a/src/engine/Graphics/Context/material.ts b/src/engine/Graphics/Context/material.ts index 2a2bbbda8..8d212551f 100644 --- a/src/engine/Graphics/Context/material.ts +++ b/src/engine/Graphics/Context/material.ts @@ -50,6 +50,7 @@ export interface MaterialOptions { * Pre-built varyings: * * * `in vec2 v_uv` - UV coordinate + * * `in vec2 v_screenuv` - UV coordinate * * Pre-built uniforms: * diff --git a/src/engine/Graphics/Context/vertex-layout.ts b/src/engine/Graphics/Context/vertex-layout.ts index 2ff4c21b1..1fd3fb214 100644 --- a/src/engine/Graphics/Context/vertex-layout.ts +++ b/src/engine/Graphics/Context/vertex-layout.ts @@ -1,7 +1,7 @@ import { Logger } from '../..'; import { Shader, VertexAttributeDefinition } from './shader'; import { VertexBuffer } from './vertex-buffer'; -import { getGlTypeSizeBytes } from './webgl-util'; +import { getGLTypeFromSource, getGlTypeSizeBytes, isAttributeInSource } from './webgl-util'; export interface VertexLayoutOptions { @@ -78,10 +78,15 @@ export class VertexLayout { return this._shader; } + private _initialized = false; + /** * Layouts need shader locations and must be bound to a shader */ initialize() { + if (this._initialized) { + return; + } if (!this._shader) { return; } @@ -95,14 +100,36 @@ export class VertexLayout { for (const attribute of this._attributes) { const attrib = shaderAttributes[attribute[0]]; if (!attrib) { - throw Error(`The attribute named: ${attribute[0]} size ${attribute[1]}`+ - ` not found in the shader source code:\n ${this._shader.vertexSource}`); + if (!isAttributeInSource(this._shader.vertexSource, attribute[0])) { + throw Error(`The attribute named: ${attribute[0]} size ${attribute[1]}`+ + ` not found in the shader source code:\n ${this._shader.vertexSource}`); + } + + this._logger.warn(`The attribute named: ${attribute[0]} size ${attribute[1]}`+ + ` not found in the compiled shader. This is possibly a bug:\n` + + ` 1. Not a bug, but should remove unused code - attribute "${attribute[0]}" is unused in` + + ` vertex/fragment and has been automatically removed by glsl compiler.\n` + + ` 2. Definitely a bug, attribute "${attribute[0]}" in layout has been mistyped or is missing` + + ` in shader, check vertex/fragment source.`); + + const glType = getGLTypeFromSource(this._gl, this._shader.vertexSource, attribute[0]); + + // Unused attribute placeholder + this._layout.push({ + name: attribute[0], + glType, + size: attribute[1], + location: -1, + normalized: false + }); } - if (attrib.size !== attribute[1]) { - throw Error(`VertexLayout size definition for attribute: [${attribute[0]}, ${attribute[1]}],` - +` doesnt match shader source size ${attrib.size}:\n ${this._shader.vertexSource}`); + if (attrib) { + if (attrib.size !== attribute[1]) { + throw Error(`VertexLayout size definition for attribute: [${attribute[0]}, ${attribute[1]}],` + +` doesn\'t match shader source size ${attrib.size}:\n ${this._shader.vertexSource}`); + } + this._layout.push(attrib); } - this._layout.push(attrib); } // calc size @@ -117,6 +144,9 @@ export class VertexLayout { this._logger.warn(`The vertex component size (${componentsPerVertex}) does NOT divide evenly into the specified vertex buffer` +` (${this._vertexBuffer.bufferData.length})`); } + + this._initialized = true; + // TODO Use VAO here instead } /** @@ -139,8 +169,10 @@ export class VertexLayout { let offset = 0; // TODO switch to VAOs if the extension is for (const vert of this._layout) { - gl.vertexAttribPointer(vert.location, vert.size, vert.glType, vert.normalized, this.totalVertexSizeBytes, offset); - gl.enableVertexAttribArray(vert.location); + if (vert.location !== -1) { // skip unused attributes + gl.vertexAttribPointer(vert.location, vert.size, vert.glType, vert.normalized, this.totalVertexSizeBytes, offset); + gl.enableVertexAttribArray(vert.location); + } offset += getGlTypeSizeBytes(gl, vert.glType) * vert.size; } } diff --git a/src/engine/Graphics/Context/webgl-util.ts b/src/engine/Graphics/Context/webgl-util.ts index 71cd9cfc1..088c4cdda 100644 --- a/src/engine/Graphics/Context/webgl-util.ts +++ b/src/engine/Graphics/Context/webgl-util.ts @@ -20,6 +20,62 @@ export function getGlTypeSizeBytes(gl: WebGLRenderingContext, type: number): num } } +/** + * Checks if an attribute is present in vertex source + */ +export function isAttributeInSource(source: string, variable: string) { + const attributeRegexTemplate = `(?[a-z0-9]+)\\s+${variable};`; + const regex = new RegExp(attributeRegexTemplate, 'g'); + const matches = regex.exec(source); + return matches?.length > 0; +} + +/** + * Attempt to discern the glType of an attribute from vertex source + * @param gl + * @param source + * @param variable + */ +export function getGLTypeFromSource(gl: WebGLRenderingContext, source: string, variable: string) { + const attributeRegexTemplate = `(?[a-z0-9]+)\\s+${variable};`; + const regex = new RegExp(attributeRegexTemplate, 'g'); + const matches = regex.exec(source); + const type = matches?.groups?.type; + + switch (type) { + case 'float': + case 'vec2': + case 'vec3': + case 'vec4': + return gl.FLOAT; + case 'int': + case 'ivec2': + case 'ivec3': + case 'ivec4': + return gl.INT; + case 'uint': + case 'uvec2': + case 'uvec3': + case 'uvec4': + return gl.UNSIGNED_INT; + case 'bool': + case 'bvec2': + case 'bvec3': + case 'bvec4': + return gl.BOOL; + case 'short': + return gl.SHORT; + case 'ushort': + return gl.UNSIGNED_SHORT; + case 'ubyte': + return gl.UNSIGNED_BYTE; + case 'byte': + return gl.BYTE; + default: + return gl.FLOAT; + } +} + /** * Based on the type return the number of attribute components diff --git a/src/spec/VertexLayoutSpec.ts b/src/spec/VertexLayoutSpec.ts index 97833b767..2793731d3 100644 --- a/src/spec/VertexLayoutSpec.ts +++ b/src/spec/VertexLayoutSpec.ts @@ -210,7 +210,7 @@ describe('A VertexLayout', () => { }).toThrowMatching((e: Error) => e.message.includes('VertexLayout size definition for attribute: [a_position, 4]')); }); - it('will calculate vertex size and webgl vbo corretly', () => { + it('will calculate vertex size and webgl vbo correctly', () => { const shader = new ex.Shader({ gl, vertexSource: `