fix(webgl): Use VertexState shader location for WebGL attributes

src/rendering/VertexState.js

@@ -42,7 +42,7 @@ export class VertexState {
 	 * @param {object} options
 	 * @param {PreferredShaderLocation[]} [options.preferredShaderLocations] If the vertex state
 	 * has "auto", null or -1 for an attribute, this will be used to determine the shader location.
-	 * If this an attribute with automatic shader location is not present in this list,
+	 * If an attribute with automatic shader location is not present in this list,
 	 * a location will be assigned that hasn't been used yet.
 	 * If this list contains the same attribute type multiple times, an error will be thrown.
 	 * If the list contains a shader location that has already been taken by the vertex state, an error will be thrown.

src/rendering/renderers/webGl/CachedMeshBufferData.js

@@ -2,6 +2,7 @@ import { Mesh } from "../../../core/Mesh.js";
 
 export class CachedMeshBufferData {
 	#meshBuffer;
+	#vertexStateBuffer;
 	#cachedMeshData;
 
 	#bufferDirty = true;
@@ -10,10 +11,12 @@ export class CachedMeshBufferData {
 
 	/**
 	 * @param {import("../../../core/MeshAttributeBuffer.js").MeshAttributeBuffer} meshBuffer
+	 * @param {import("../../VertexStateBuffer.js").VertexStateBuffer} vertexStateBuffer
 	 * @param {import("./CachedMeshData.js").CachedMeshData} meshData
 	 */
-	constructor(meshBuffer, meshData) {
+	constructor(meshBuffer, vertexStateBuffer, meshData) {
 		this.#meshBuffer = meshBuffer;
+		this.#vertexStateBuffer = vertexStateBuffer;
 		this.#cachedMeshData = meshData;
 
 		meshBuffer.onBufferChanged(this.#onBufferChanged);
@@ -54,6 +57,7 @@ export class CachedMeshBufferData {
 		}
 
 		const attributes = [];
+		let i = 0;
 		for (const attributeSettings of this.#meshBuffer.attributeSettings) {
 			let type;
 			const normalized = false;
@@ -66,12 +70,19 @@ export class CachedMeshBufferData {
 			} else {
 				throw new Error("Mesh has an unsupported attribute format");
 			}
+			const vertexStateAttribute = this.#vertexStateBuffer.attributes[i];
+			const shaderLocation = vertexStateAttribute.shaderLocation;
+			if (shaderLocation == null || shaderLocation == "auto" || shaderLocation < 0) {
+				throw new Error("Automatic shader locations are not supported in the webgl renderer.");
+			}
 			attributes.push({
 				componentCount: attributeSettings.componentCount,
 				type,
 				normalized,
 				offset: 0, // TODO
+				shaderLocation,
 			});
+			i++;
 		}
 
 		return {

src/rendering/renderers/webGl/CachedMeshData.js

@@ -21,12 +21,19 @@ export class CachedMeshData {
 	constructor(mesh, renderer) {
 		this.#mesh = mesh;
 		this.#renderer = renderer;
+		const vertexState = mesh.vertexState;
+		if (!vertexState) {
+			throw new Error("Assertion failed, mesh has no vertex state");
+		}
 
 		// todo: remove old bufferdata when the list of buffers changes
 		this.#buffers = [];
+		let i = 0;
 		for (const meshBuffer of mesh.getAttributeBuffers(false)) {
-			const bufferData = new CachedMeshBufferData(meshBuffer, this);
+			const vertexStateBuffer = vertexState.buffers[i];
+			const bufferData = new CachedMeshBufferData(meshBuffer, vertexStateBuffer, this);
 			this.#buffers.push(bufferData);
+			i++;
 		}
 
 		this.createIndexGpuBuffer();

src/rendering/renderers/webGl/CachedProgramData.js

@@ -7,8 +7,13 @@
  * @property {WebGLUniformLocation?} mvpMatrix
  */
 
+import { parseAttributeLocations as parseTaggedAttributeLocations } from "./glslParsing.js";
+
 export class CachedProgramData {
 	#program;
+	get program() {
+		return this.#program;
+	}
 
 	/** @type {ViewUniformLocations?} */
 	#viewUniformLocations = null;
@@ -18,10 +23,35 @@ export class CachedProgramData {
 	/** @type {Map<string, WebGLUniformLocation?>} */
 	#materialUniformLocations = new Map();
 
+	/** @type {Map<number, string>} */
+	#taggedAttributeLocations = new Map();
+
 	/**
-	 * @param {WebGLProgram} program
+	 * @param {WebGLRenderingContext} gl
+	 * @param {import("../../ShaderSource.js").ShaderSource} vertexShaderSource
+	 * @param {import("../../ShaderSource.js").ShaderSource} fragmentShaderSource
+	 * @param {WebGLShader} vertexShader
+	 * @param {WebGLShader} fragmentShader
 	 */
-	constructor(program) {
+	constructor(gl, vertexShaderSource, fragmentShaderSource, vertexShader, fragmentShader) {
+		const program = gl.createProgram();
+		if (!program) throw new Error("Failed to create program");
+
+		const taggedAttributeLocations = parseTaggedAttributeLocations(vertexShaderSource.source);
+		for (const { identifier, location } of taggedAttributeLocations) {
+			if (this.#taggedAttributeLocations.has(location)) {
+				throw new Error(`Shader contains multiple attributes tagged with @location(${location}).`);
+			}
+			this.#taggedAttributeLocations.set(location, identifier);
+		}
+
+		gl.attachShader(program, vertexShader);
+		gl.attachShader(program, fragmentShader);
+		gl.linkProgram(program);
+		if (!gl.getProgramParameter(program, gl.LINK_STATUS)) {
+			throw new Error(`Failed to link shader program: ${gl.getProgramInfoLog(program)}`);
+		}
+
 		this.#program = program;
 	}
 
@@ -62,4 +92,24 @@ export class CachedProgramData {
 		this.#materialUniformLocations.set(name, location);
 		return location;
 	}
+
+	/**
+	 * @param {WebGLRenderingContext} gl
+	 * @param {number} taggedShaderLocation The id that the attribute was tagged
+	 * with in the shader using a `@location` comment.
+	 */
+	getAttribLocation(gl, taggedShaderLocation) {
+		const identifier = this.#taggedAttributeLocations.get(taggedShaderLocation);
+		if (!identifier) {
+			// If no identifier with this shader location was found in the vertex shader, this could either be because:
+			// - the user forgot to tag it with a @location comment
+			// - or because the attribute is not used at all.
+			// In the first case we should ideally throw an error, in the second case we should do nothing.
+			// However, there's no easy way for us to detect if an attribute is unused, so we'll just
+			// return -1, this will cause the renderer to not bind the attribute buffer.
+			return -1;
+		}
+
+		return gl.getAttribLocation(this.#program, identifier);
+	}
 }

src/rendering/renderers/webGl/WebGlRenderer.js

@@ -42,16 +42,13 @@ export class WebGlRenderer extends Renderer {
 	/** @type {WeakMap<import("../../Material.js").Material, CachedMaterialData>} */
 	#cachedMaterialData = new WeakMap();
 
-	/** @type {WeakMap<WebGLProgram, CachedProgramData>} */
-	#cachedProgramData = new WeakMap();
-
 	/** @type {WeakMap<import("../../../core/Mesh.js").Mesh, CachedMeshData>} */
 	#cachedMeshDatas = new WeakMap();
 
 	/** @type {MultiKeyWeakMap<[number, import("../../ShaderSource.js").ShaderSource], WebGLShader>} */
 	#cachedShaders = new MultiKeyWeakMap([], { allowNonObjects: true });
 
-	/** @type {MultiKeyWeakMap<[import("../../ShaderSource.js").ShaderSource, import("../../ShaderSource.js").ShaderSource], WebGLProgram>} */
+	/** @type {MultiKeyWeakMap<[import("../../ShaderSource.js").ShaderSource, import("../../ShaderSource.js").ShaderSource], CachedProgramData>} */
 	#cachedPrograms = new MultiKeyWeakMap();
 
 	/** @type {OES_element_index_uint?} */
@@ -197,7 +194,7 @@ export class WebGlRenderer extends Renderer {
 
 		/**
 		 * @typedef MaterialConfigRenderData
-		 * @property {Map<WebGLProgram, MaterialRenderData>} materialRenderDatas
+		 * @property {Map<CachedProgramData, MaterialRenderData>} materialRenderDatas
 		 */
 
 		// Group all meshes by material config
@@ -212,7 +209,7 @@ export class WebGlRenderer extends Renderer {
 				const materialConfig = materialData.getMaterialConfig();
 				if (!materialConfig || !materialConfig.vertexShader || !materialConfig.fragmentShader) continue;
 
-				const program = this.#getProgram(materialConfig.vertexShader, materialConfig.fragmentShader);
+				const program = this.#getCachedProgramData(materialConfig.vertexShader, materialConfig.fragmentShader);
 
 				let programRenderData = materialConfigRenderDatas.get(materialConfig);
 				if (!programRenderData) {
@@ -247,9 +244,8 @@ export class WebGlRenderer extends Renderer {
 		});
 
 		for (const [materialConfig, programRenderData] of sortedProgramRenderDatas) {
-			for (const [program, materialRenderData] of programRenderData.materialRenderDatas) {
-				gl.useProgram(program);
-				const programData = this.#getCachedProgramData(program);
+			for (const [programData, materialRenderData] of programRenderData.materialRenderDatas) {
+				gl.useProgram(programData.program);
 				const viewUniformLocations = programData.getViewUniformLocations(gl);
 				const modelUniformLocations = programData.getModelUniformLocations(gl);
 
@@ -295,6 +291,7 @@ Material.setProperty("${mappedData.mappedName}", customData)`;
 					for (const { component: meshComponent, worldMatrix } of meshRenderDatas) {
 						const mesh = meshComponent.mesh;
 						if (!mesh) continue;
+						if (!mesh.vertexState) continue;
 
 						if (modelUniformLocations.mvpMatrix) {
 							const mvpMatrix = Mat4.multiplyMatrices(worldMatrix, viewProjectionMatrix);
@@ -317,19 +314,20 @@ Material.setProperty("${mappedData.mappedName}", customData)`;
 							}
 							gl.bindBuffer(gl.ELEMENT_ARRAY_BUFFER, indexBufferData.buffer);
 
-							let i = 0;
 							for (const { buffer, attributes, stride } of meshData.getAttributeBufferData()) {
 								gl.bindBuffer(gl.ARRAY_BUFFER, buffer);
-								for (const { componentCount, type, normalized, offset } of attributes) {
-									gl.vertexAttribPointer(i, componentCount, type, normalized, stride, offset);
-									gl.enableVertexAttribArray(i);
-									i++;
+								for (const { shaderLocation, componentCount, type, normalized, offset } of attributes) {
+									const index = programData.getAttribLocation(gl, shaderLocation);
+									if (index >= 0) {
+										gl.vertexAttribPointer(index, componentCount, type, normalized, stride, offset);
+										gl.enableVertexAttribArray(index);
+									}
 								}
 							}
 
 							gl.drawElements(gl.TRIANGLES, indexBufferData.count, indexFormat, 0);
 						} else {
-						// TODO
+							// TODO
 						}
 					}
 				}
@@ -354,18 +352,6 @@ Material.setProperty("${mappedData.mappedName}", customData)`;
 		return data;
 	}
 
-	/**
-	 * @param {WebGLProgram} program
-	 */
-	#getCachedProgramData(program) {
-		let data = this.#cachedProgramData.get(program);
-		if (!data) {
-			data = new CachedProgramData(program);
-			this.#cachedProgramData.set(program, data);
-		}
-		return data;
-	}
-
 	/**
 	 * @param {import("../../../core/Mesh.js").Mesh} mesh
 	 */
@@ -405,7 +391,7 @@ Material.setProperty("${mappedData.mappedName}", customData)`;
 	 * @param {import("../../ShaderSource.js").ShaderSource} vertexShaderSource
 	 * @param {import("../../ShaderSource.js").ShaderSource} fragmentShaderSource
 	 */
-	#getProgram(vertexShaderSource, fragmentShaderSource) {
+	#getCachedProgramData(vertexShaderSource, fragmentShaderSource) {
 		const existing = this.#cachedPrograms.get([vertexShaderSource, fragmentShaderSource]);
 		if (existing) return existing;
 
@@ -415,18 +401,9 @@ Material.setProperty("${mappedData.mappedName}", customData)`;
 		const vertexShader = this.#getShader(vertexShaderSource, gl.VERTEX_SHADER);
 		const fragmentShader = this.#getShader(fragmentShaderSource, gl.FRAGMENT_SHADER);
 
-		const program = gl.createProgram();
-		if (!program) throw new Error("Failed to create program");
-
-		gl.attachShader(program, vertexShader);
-		gl.attachShader(program, fragmentShader);
-		gl.linkProgram(program);
-		if (!gl.getProgramParameter(program, gl.LINK_STATUS)) {
-			throw new Error(`Failed to link shader program: ${gl.getProgramInfoLog(program)}`);
-		}
-
-		this.#cachedPrograms.set([vertexShaderSource, fragmentShaderSource], program);
-		return program;
+		const cachedProgramData = new CachedProgramData(gl, vertexShaderSource, fragmentShaderSource, vertexShader, fragmentShader);
+		this.#cachedPrograms.set([vertexShaderSource, fragmentShaderSource], cachedProgramData);
+		return cachedProgramData;
 	}
 
 	/**

src/rendering/renderers/webGl/glslParsing.js

@@ -0,0 +1,60 @@
+/**
+ * Regex string for matching glsl identifiers according to the glsl spec:
+ * https://registry.khronos.org/OpenGL/specs/gl/GLSLangSpec.4.60.html#identifiers
+ * @param {string} group
+ */
+export const identifierRegex = "(?:[a-zA-Z_][0-9a-zA-Z_]*)";
+
+/**
+ * @typedef ParsedAttributeLocation
+ * @property {string} identifier The name of the attribute as it appears in the shader.
+ * @property {number} location The shader location that the identifier was tagged with.
+ */
+
+/**
+ * Finds all attributes in a shader and the value of the `@location` comment they are tagged with.
+ * @param {string} shaderSource
+ * @returns {ParsedAttributeLocation[]}
+ */
+export function parseAttributeLocations(shaderSource) {
+	// This loosely follows
+	// https://registry.khronos.org/OpenGL/specs/gl/GLSLangSpec.4.60.html#shading-language-grammar:~:text=conditional_expression-,declaration%20%3A,-function_prototype%20SEMICOLON%0Ainit_declarator_list
+	let attributesRegex = "";
+	// Capture the location tag
+	attributesRegex += "@location\\s*\\(\\s*(?<location>\\d+)\\s*\\)";
+	// Allow whitespace or any other tags after the line that contains the location tag
+	attributesRegex += ".*";
+	// Only one new line allowed
+	attributesRegex += "\\n";
+	// Allow whitespace before the attribute keyword
+	attributesRegex += "\\s*";
+	// Attribute storage qualifier
+	attributesRegex += "attribute";
+	// any additional `type_qualifier`s
+	attributesRegex += ".*";
+	// at least one whitespace
+	attributesRegex += "\\s";
+	// Capture the IDENTIFIER
+	attributesRegex += `(?<identifier>${identifierRegex})`;
+	// whitespace
+	attributesRegex += "\\s*";
+	// SEMICOLON
+	attributesRegex += ";";
+
+	/** @type {ParsedAttributeLocation[]} */
+	const parsedLocations = [];
+
+	for (const match of shaderSource.matchAll(new RegExp(attributesRegex, "g"))) {
+		if (!match.groups) continue;
+		const identifier = match.groups.identifier;
+		if (!identifier) continue;
+		const location = match.groups.location;
+		if (!location) continue;
+		parsedLocations.push({
+			identifier,
+			location: parseInt(location, 10),
+		});
+	}
+
+	return parsedLocations;
+}

src/util/wgslParsing.js

@@ -200,8 +200,7 @@ export function parseBindings(shaderSource) {
 /**
  * @typedef ParsedVertexInputProperty
  * @property {string} identifier The name of the binding as it appears in the shader.
- * @property {number} location The shader location that should be used when the vertex state
- * has a shader location set to 'auto'.
+ * @property {number} location The shader location that the identifier was tagged with.
  */
 
 /**

test/unit/src/rendering/renderers/shared/sceneUtil.js

@@ -19,6 +19,7 @@ export function createVertexState() {
 						componentCount: 3,
 						format: Mesh.AttributeFormat.FLOAT32,
 						unsigned: false,
+						shaderLocation: 0,
 					},
 				],
 			},
@@ -31,6 +32,7 @@ export function createVertexState() {
 						componentCount: 4,
 						format: Mesh.AttributeFormat.FLOAT32,
 						unsigned: false,
+						shaderLocation: 1,
 					},
 				],
 			},
@@ -43,7 +45,7 @@ export function createVertexState() {
  * @param {object} options
  * @param {Entity} options.scene
  * @param {import("../../../../../../src/mod.js").Material} options.material
- * @param {VertexState} options.vertexState
+ * @param {VertexState?} options.vertexState
  */
 export function createCubeEntity({ scene, material, vertexState }) {
 	const cubeEntity = scene.add(new Entity("cube"));