Skip to content

Commit

Permalink
fix: Firefox + Linux custom shader issue with glsl compiler attribute…
Browse files Browse the repository at this point in the history
… 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)
  • Loading branch information
eonarheim authored Feb 29, 2024
1 parent 2acaeb2 commit f913222
Show file tree
Hide file tree
Showing 5 changed files with 100 additions and 10 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
1 change: 1 addition & 0 deletions src/engine/Graphics/Context/material.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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:
*
Expand Down
50 changes: 41 additions & 9 deletions src/engine/Graphics/Context/vertex-layout.ts
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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;
}
Expand All @@ -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
Expand All @@ -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
}

/**
Expand All @@ -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;
}
}
Expand Down
56 changes: 56 additions & 0 deletions src/engine/Graphics/Context/webgl-util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = `(?<type>[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 = `(?<type>[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
Expand Down
2 changes: 1 addition & 1 deletion src/spec/VertexLayoutSpec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: `
Expand Down

0 comments on commit f913222

Please sign in to comment.