Repository: playcanvas/engine
Pull request #6253 (2024-04-15 11:39:02)

Updated handling of defines specified by a shader definition
Change #6250 introduced shader handling a map of includes and defines. The defines map was directly passed to the preprocessor, which used it internally when handling `#ifdef` type of code.
As our preprocessor does not handle generic defines, for example `#define LOD_LEVEL 4` which can then be used anywhere in the following code, the implementation has changed to allow this.

This PR converts all defines specified in the define map to source code, and this is attached to the shader source code directly. This allows us to pass on processing of those defines to the actual platforms that compiles the shader.


Patch:
diff --git a/src/core/preprocessor.js b/src/core/preprocessor.js
index 43490ffc7..b2dd54370 100644
--- a/src/core/preprocessor.js
+++ b/src/core/preprocessor.js
@@ -44,17 +44,12 @@ class Preprocessor {
      * Run c-like preprocessor on the source code, and resolves the code based on the defines and ifdefs
      *
      * @param {string} source - The source code to work on.
-     * @param {Map<string, string>} [defines] - A map containing key-value pairs of define names
-     * and their values. These are used for resolving #ifdef style of directives in the source.
      * @param {Map<string, string>} [includes] - A map containing key-value pairs of include names
      * and their content. These are used for resolving #include directives in the source.
      * @param {boolean} [stripUnusedColorAttachments] - If true, strips unused color attachments.
      * @returns {string|null} Returns preprocessed source code, or null in case of error.
      */
-    static run(source, defines, includes = new Map(), stripUnusedColorAttachments = false) {
-
-        // shallow clone as we will modify the map
-        defines = defines ? new Map(defines) : new Map();
+    static run(source, includes = new Map(), stripUnusedColorAttachments = false) {
 
         // strips comments, handles // and many cases of /*
         source = source.replace(/\/\*[\s\S]*?\*\/|([^\\:]|^)\/\/.*$/gm, '$1');
@@ -65,6 +60,7 @@ class Preprocessor {
             .join('\n');
 
         // generate defines to remove unused color attachments
+        const defines = new Map();
         if (stripUnusedColorAttachments) {
 
             // find out how many times pcFragColorX is used (see gles3.js)
diff --git a/src/platform/graphics/shader-utils.js b/src/platform/graphics/shader-utils.js
index c35bb59ae..d60eca621 100644
--- a/src/platform/graphics/shader-utils.js
+++ b/src/platform/graphics/shader-utils.js
@@ -44,24 +44,22 @@ class ShaderUtils {
      * @param {object} [options.attributes] - Attributes. Will be extracted from the vertexCode if
      * not provided.
      * @param {string} options.vertexCode - The vertex shader code.
-     * @param {string} [options.vertexDefines] - The vertex shader defines.
      * @param {string} [options.vertexExtensions] - The vertex shader extensions code.
      * @param {string} [options.fragmentCode] - The fragment shader code.
-     * @param {string} [options.fragmentDefines] - The fragment shader defines.
      * @param {string} [options.fragmentExtensions] - The fragment shader extensions code.
      * @param {string} [options.fragmentPreamble] - The preamble string for the fragment shader.
      * @param {boolean} [options.useTransformFeedback] - Whether to use transform feedback. Defaults
      * to false.
-     * @param {Map<string, string>} [options.vertexIncludesMap] - A map containing key-value pairs of
+     * @param {Map<string, string>} [options.vertexIncludes] - A map containing key-value pairs of
      * include names and their content. These are used for resolving #include directives in the
      * vertex shader source.
-     * @param {Map<string, string>} [options.vertexDefinesMap] - A map containing key-value pairs of
+     * @param {Map<string, string>} [options.vertexDefines] - A map containing key-value pairs of
      * define names and their values. These are used for resolving #ifdef style of directives in the
      * vertex code.
-     * @param {Map<string, string>} [options.fragmentIncludesMap] - A map containing key-value pairs
+     * @param {Map<string, string>} [options.fragmentIncludes] - A map containing key-value pairs
      * of include names and their content. These are used for resolving #include directives in the
      * fragment shader source.
-     * @param {Map<string, string>} [options.fragmentDefinesMap] - A map containing key-value pairs of
+     * @param {Map<string, string>} [options.fragmentDefines] - A map containing key-value pairs of
      * define names and their values. These are used for resolving #ifdef style of directives in the
      * fragment code.
      * @param {string | string[]} [options.fragmentOutputTypes] - Fragment shader output types,
@@ -71,6 +69,10 @@ class ShaderUtils {
      */
     static createDefinition(device, options) {
         Debug.assert(options);
+        Debug.assert(!options.vertexDefines || options.vertexDefines instanceof Map);
+        Debug.assert(!options.vertexIncludes || options.vertexIncludes instanceof Map);
+        Debug.assert(!options.fragmentDefines || options.fragmentDefines instanceof Map);
+        Debug.assert(!options.fragmentIncludes || options.fragmentIncludes instanceof Map);
 
         const getDefines = (gpu, gl2, gl1, isVertex, options) => {
 
@@ -101,18 +103,18 @@ class ShaderUtils {
         const name = options.name ?? 'Untitled';
 
         // vertex code
-        const vertDefines = options.vertexDefines || getDefines(webgpuVS, gles3VS, '', true, options);
         const vertCode = ShaderUtils.versionCode(device) +
-            vertDefines +
+            getDefines(webgpuVS, gles3VS, '', true, options) +
+            ShaderUtils.getDefinesCode(options.vertexDefines) +
             sharedFS +
             ShaderUtils.getShaderNameCode(name) +
             options.vertexCode;
 
         // fragment code
-        const fragDefines = options.fragmentDefines || getDefines(webgpuFS, gles3FS, gles2FS, false, options);
         const fragCode = (options.fragmentPreamble || '') +
             ShaderUtils.versionCode(device) +
-            fragDefines +
+            getDefines(webgpuFS, gles3FS, gles2FS, false, options) +
+            ShaderUtils.getDefinesCode(options.fragmentDefines) +
             ShaderUtils.precisionCode(device) + '\n' +
             sharedFS +
             ShaderUtils.getShaderNameCode(name) +
@@ -125,15 +127,26 @@ class ShaderUtils {
             name: name,
             attributes: attribs,
             vshader: vertCode,
-            vdefines: options.vertexDefinesMap,
-            vincludes: options.vertexIncludesMap,
-            fdefines: options.fragmentDefinesMap,
-            fincludes: options.fragmentIncludesMap,
+            vincludes: options.vertexIncludes,
+            fincludes: options.fragmentIncludes,
             fshader: fragCode,
             useTransformFeedback: options.useTransformFeedback
         };
     }
 
+    /**
+     * @param {Map<string, string>} [defines] - A map containing key-value pairs.
+     * @returns {string} The shader code for the defines.
+     * @private
+     */
+    static getDefinesCode(defines) {
+        let code = '';
+        defines?.forEach((value, key) => {
+            code += `#define ${key} ${value}\n`;
+        });
+        return code;
+    }
+
     // SpectorJS integration
     static getShaderNameCode(name) {
         return `#define SHADER_NAME ${name}\n`;
diff --git a/src/platform/graphics/shader.js b/src/platform/graphics/shader.js
index e7266a8ba..b3692897a 100644
--- a/src/platform/graphics/shader.js
+++ b/src/platform/graphics/shader.js
@@ -56,15 +56,9 @@ class Shader {
      * @param {Map<string, string>} [definition.vincludes] - A map containing key-value pairs of
      * include names and their content. These are used for resolving #include directives in the
      * vertex shader source.
-     * @param {Map<string, string>} [definition.vdefines] - A map containing key-value pairs of
-     * define names and their values. These are used for resolving #ifdef style of directives in the
-     * vertex code.
      * @param {Map<string, string>} [definition.fincludes] - A map containing key-value pairs
      * of include names and their content. These are used for resolving #include directives in the
      * fragment shader source.
-     * @param {Map<string, string>} [definition.fdefines] - A map containing key-value pairs of
-     * define names and their values. These are used for resolving #ifdef style of directives in the
-     * fragment code.
      * @param {boolean} [definition.useTransformFeedback] - Specifies that this shader outputs
      * post-VS data to a buffer.
      * @param {string | string[]} [definition.fragmentOutputTypes] - Fragment shader output types,
@@ -118,13 +112,13 @@ class Shader {
             Debug.assert(definition.fshader, 'No fragment shader has been specified when creating a shader.');
 
             // pre-process shader sources
-            definition.vshader = Preprocessor.run(definition.vshader, definition.vdefines, definition.vincludes);
+            definition.vshader = Preprocessor.run(definition.vshader, definition.vincludes);
 
             // Strip unused color attachments from fragment shader.
             // Note: this is only needed for iOS 15 on WebGL2 where there seems to be a bug where color attachments that are not
             // written to generate metal linking errors. This is fixed on iOS 16, and iOS 14 does not support WebGL2.
             const stripUnusedColorAttachments = graphicsDevice.isWebGL2 && (platform.name === 'osx' || platform.name === 'ios');
-            definition.fshader = Preprocessor.run(definition.fshader, definition.fdefines, definition.fincludes, stripUnusedColorAttachments);
+            definition.fshader = Preprocessor.run(definition.fshader, definition.fincludes, stripUnusedColorAttachments);
         }
 
         this.impl = graphicsDevice.createShaderImpl(this);
diff --git a/src/scene/shader-lib/programs/basic.js b/src/scene/shader-lib/programs/basic.js
index b00868015..87227ac87 100644
--- a/src/scene/shader-lib/programs/basic.js
+++ b/src/scene/shader-lib/programs/basic.js
@@ -169,8 +169,8 @@ class ShaderGeneratorBasic extends ShaderGenerator {
         if (options.diffuseMap) defines.set('DIFFUSE_MAP', true);
 
         definitionOptions.vertexCode = vShader;
-        definitionOptions.vertexIncludesMap = includes;
-        definitionOptions.vertexDefinesMap = defines;
+        definitionOptions.vertexIncludes = includes;
+        definitionOptions.vertexDefines = defines;
     }
 
     createFragmentDefinition(definitionOptions, options, shaderPassInfo) {
@@ -189,8 +189,8 @@ class ShaderGeneratorBasic extends ShaderGenerator {
         if (options.alphaTest) defines.set('ALPHA_TEST', true);
 
         definitionOptions.fragmentCode = fShader;
-        definitionOptions.fragmentIncludesMap = includes;
-        definitionOptions.fragmentDefinesMap = defines;
+        definitionOptions.fragmentIncludes = includes;
+        definitionOptions.fragmentDefines = defines;
     }
 
     createShaderDefinition(device, options) {


Test patch:
diff --git a/test/core/preprocessor.test.mjs b/test/core/preprocessor.test.mjs
index 2995abe6b..79503ca4b 100644
--- a/test/core/preprocessor.test.mjs
+++ b/test/core/preprocessor.test.mjs
@@ -14,8 +14,6 @@ describe('Preprocessor', function () {
         ['inc2', 'block2']
     ]);
 
-    const defines = new Map();
-
     const srcData = `
         
         #define FEATURE1
@@ -78,82 +76,82 @@ describe('Preprocessor', function () {
     `;
 
     it('returns false for MORPH_A', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('MORPH_A')).to.equal(false);
+        expect(Preprocessor.run(srcData, includes).includes('MORPH_A')).to.equal(false);
     });
 
     it('returns false for MORPH_B', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('MORPH_B')).to.equal(false);
+        expect(Preprocessor.run(srcData, includes).includes('MORPH_B')).to.equal(false);
     });
 
     it('returns true for $', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('$')).to.equal(true);
+        expect(Preprocessor.run(srcData, includes).includes('$')).to.equal(true);
     });
 
     it('returns true for TEST1', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('TEST1')).to.equal(true);
+        expect(Preprocessor.run(srcData, includes).includes('TEST1')).to.equal(true);
     });
 
     it('returns true for TEST2', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('TEST2')).to.equal(true);
+        expect(Preprocessor.run(srcData, includes).includes('TEST2')).to.equal(true);
     });
 
     it('returns true for TEST3', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('TEST3')).to.equal(true);
+        expect(Preprocessor.run(srcData, includes).includes('TEST3')).to.equal(true);
     });
 
     it('returns true for TEST4', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('TEST4')).to.equal(true);
+        expect(Preprocessor.run(srcData, includes).includes('TEST4')).to.equal(true);
     });
 
     it('returns false for TEST5', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('TEST5')).to.equal(false);
+        expect(Preprocessor.run(srcData, includes).includes('TEST5')).to.equal(false);
     });
 
     it('returns true for TEST6', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('TEST6')).to.equal(true);
+        expect(Preprocessor.run(srcData, includes).includes('TEST6')).to.equal(true);
     });
 
     it('returns false for TEST7', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('TEST7')).to.equal(false);
+        expect(Preprocessor.run(srcData, includes).includes('TEST7')).to.equal(false);
     });
 
     it('returns false for TEST8', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('TEST8')).to.equal(false);
+        expect(Preprocessor.run(srcData, includes).includes('TEST8')).to.equal(false);
     });
 
     it('returns false for TEST9', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('TEST9')).to.equal(false);
+        expect(Preprocessor.run(srcData, includes).includes('TEST9')).to.equal(false);
     });
 
     it('returns true for TEST10', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('TEST10')).to.equal(true);
+        expect(Preprocessor.run(srcData, includes).includes('TEST10')).to.equal(true);
     });
 
     it('returns false for TEST11', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('TEST11')).to.equal(false);
+        expect(Preprocessor.run(srcData, includes).includes('TEST11')).to.equal(false);
     });
 
     it('returns false for TEST12', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('TEST12')).to.equal(false);
+        expect(Preprocessor.run(srcData, includes).includes('TEST12')).to.equal(false);
     });
 
     it('returns true for TEST13', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('TEST13')).to.equal(true);
+        expect(Preprocessor.run(srcData, includes).includes('TEST13')).to.equal(true);
     });
 
     it('returns false for TEST14', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('TEST14')).to.equal(false);
+        expect(Preprocessor.run(srcData, includes).includes('TEST14')).to.equal(false);
     });
 
     it('returns true for INC1', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('block1')).to.equal(true);
+        expect(Preprocessor.run(srcData, includes).includes('block1')).to.equal(true);
     });
 
     it('returns false for INC2', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('block2')).to.equal(false);
+        expect(Preprocessor.run(srcData, includes).includes('block2')).to.equal(false);
     });
 
     it('returns true for nested', function () {
-        expect(Preprocessor.run(srcData, defines, includes).includes('nested')).to.equal(true);
+        expect(Preprocessor.run(srcData, includes).includes('nested')).to.equal(true);
     });
 });