Repository: playcanvas/engine
Pull request #4156 (2022-03-31 12:46:43)

Preprocessor to resolve ifdefs is applied to shader programs
A new private class is added to preprocess defines and ifdefs in the shader with the goal of removing unused code. This is a first preparation step. Following steps will further modify the shader code, in order to convert uniforms to uniform blocks.

**New (private) API:**
```
/**
 * Run c-like proprocessor on the source code, and resolves the code based on the defines and ifdefs
 *
 * @param {string} source - The source code to work on.
 * @returns {string|null} Returns preprocessed source code.
 */
static Preprocessor.run(source) {
```

**Note** - Simple expression evaluation only is supported for ifdefs, handles cases:
- expression
- defined(expression)
- !defined(expression)
 
 It does not handle more complex cases, which would require more complex system:
 - defined(A) || defined(B)
And so few use cases in the shaders were converted to the supported format.

Unit test has also been added.

Additionally, Debug logging functionality has been extended with **trace** function.
**New (private) API (and also examples of use):**
```
Debug.setTrace(`Preprocessor`, true);
Debug.trace(`Preprocessor`, "message")
```

and this is displayed in the console:
`[Preprocessor]  message`

yaustar: Does this affect the sourcemap generation?
mvaligursky: > Does this affect the sourcemap generation?

This is only used to process shaders in memory .. so no.
willeastcott: This is definitely something for @slimbuck to review - I'll pass.
mvaligursky: Updates based on comments @slimbuck 
@willeastcott - could you please look at the Debug.trace part I added.

Patch:
diff --git a/rollup.config.js b/rollup.config.js
index adf8917ca..c013a4997 100644
--- a/rollup.config.js
+++ b/rollup.config.js
@@ -136,6 +136,7 @@ const stripOptions = {
         'Debug.warn',
         'Debug.error',
         'Debug.log',
+        'Debug.trace',
         'DebugGraphics.pushGpuMarker',
         'DebugGraphics.popGpuMarker',
         'WorldClustersDebug.render'
diff --git a/src/core/debug.js b/src/core/debug.js
index 1d6f4d897..2b4d2b3bd 100644
--- a/src/core/debug.js
+++ b/src/core/debug.js
@@ -13,6 +13,31 @@ class Debug {
      */
     static _deprecatedMessages = new Set();
 
+    /**
+     * Set storing names of enabled trace channels
+     *
+     * @type {Set<string>}
+     * @private
+     */
+    static _traceChannels = new Set();
+
+    /**
+     * Enable or disable trace channel
+     *
+     * @param {string} channel - Name of the trace channel
+     * @param {boolean} enabled - new enabled state for it
+     */
+    static setTrace(channel, enabled = true) {
+
+        // #if _DEBUG
+        if (enabled) {
+            Debug._traceChannels.add(channel);
+        } else {
+            Debug._traceChannels.delete(channel);
+        }
+        // #endif
+    }
+
     /**
      * Deprecated warning message.
      *
@@ -28,7 +53,7 @@ class Debug {
     /**
      * Assertion error message. If the assertion is false, the error message is written to the log.
      *
-     * @param {boolean} assertion - The assertion to check.
+     * @param {boolean|object} assertion - The assertion to check.
      * @param {...*} args - The values to be written to the log.
      */
     static assert(assertion, ...args) {
@@ -63,6 +88,18 @@ class Debug {
     static error(...args) {
         console.error(...args);
     }
+
+    /**
+     * Trace message, which is logged to the console if the tracing for the channel is enabled
+     *
+     * @param {string} channel - The trace channel
+     * @param {...*} args - The values to be written to the log.
+     */
+    static trace(channel, ...args) {
+        if (Debug._traceChannels.has(channel)) {
+            console.log(`[${channel}] `, ...args);
+        }
+    }
 }
 
 export { Debug };
diff --git a/src/core/preprocessor.js b/src/core/preprocessor.js
new file mode 100644
index 000000000..e91ef8973
--- /dev/null
+++ b/src/core/preprocessor.js
@@ -0,0 +1,277 @@
+import { Debug } from './debug.js';
+
+// id for debug tracing
+const TRACEID = 'Preprocessor';
+
+// accepted keywords
+const KEYWORD = /([ ]*)+#(ifn?def|if|endif|else|elif|define|undef)/g;
+
+// #define EXPRESSION
+const DEFINE = /define[ ]+([^\n]+)\r?(?:\n|$)/g;
+
+// #undef EXPRESSION
+const UNDEF = /undef[ ]+([^\n]+)\r?(?:\n|$)/g;
+
+// #ifdef/#ifndef SOMEDEFINE, #if EXPRESSION
+const IF = /(ifdef|ifndef|if)[ ]*([^\r\n]+)\r?\n/g;
+
+// #endif/#else or #elif EXPRESSION
+const ENDIF = /(endif|else|elif)([ ]+[^\r\n]+)?\r?(?:\n|$)/g;
+
+// identifier
+const IDENTIFIER = /([\w-]+)/;
+
+// [!]defined(EXPRESSION)
+const DEFINED = /(!|\s)?defined\(([\w-]+)\)/;
+
+// currently unsupported characters in the expression: | & < > = + -
+const INVALID = /[><=|&+-]/g;
+
+/**
+ * Pure static class implementing subset of C-style preprocessor.
+ * inspired by: https://github.com/dcodeIO/Preprocessor.js
+ *
+ * @ignore
+ */
+class Preprocessor {
+    /**
+     * Run c-like proprocessor on the source code, and resolves the code based on the defines and ifdefs
+     *
+     * @param {string} source - The source code to work on.
+     * @returns {string|null} Returns preprocessed source code, or null in case of error.
+     */
+    static run(source) {
+
+        // strips comments, handles // and many cases of /*
+        source = source.replace(/\/\*[\s\S]*?\*\/|([^\\:]|^)\/\/.*$/gm, '$1');
+
+        // right trim each line
+        source = source.split(/\r?\n/)
+            .map(line => line.trimEnd())
+            .join('\n');
+
+        // proprocess defines / ifdefs ..
+        source = this._preprocess(source);
+
+        if (source !== null) {
+            // convert lines with only white space into empty string
+            source = source.split(/\r?\n/)
+                .map(line => (line.trim() === '' ? '' : line))
+                .join('\n');
+
+            // remove more than 1 consecutive empty lines
+            source = source.replace(/(\n\n){3,}/gm, '\n\n');
+        }
+
+        return source;
+    }
+
+    static _preprocess(source) {
+
+        const originalSource = source;
+
+        // stack, storing info about ifdef blocks
+        const stack = [];
+
+        // true if the function encounter a problem
+        let error = false;
+
+        // active defines, maps define name to its value
+        /** @type {Map<string, string>} */
+        const defines = new Map();
+
+        let match;
+        while ((match = KEYWORD.exec(source)) !== null) {
+
+            const keyword = match[2];
+            switch (keyword) {
+                case 'define': {
+
+                    // read the rest of the define line
+                    DEFINE.lastIndex = match.index;
+                    const define = DEFINE.exec(source);
+                    Debug.assert(define, `Invalid [${keyword}]: ${source.substring(match.index, match.index + 100)}...`);
+                    error ||= define === null;
+                    const expression = define[1];
+
+                    // split it to identifier name and a value
+                    IDENTIFIER.lastIndex = define.index;
+                    const identifierValue = IDENTIFIER.exec(expression);
+                    const identifier = identifierValue[1];
+                    let value = expression.substring(identifier.length).trim();
+                    if (value === "") value = "true";
+
+                    // are we inside if-blocks that are accepted
+                    const keep = Preprocessor._keep(stack);
+
+                    if (keep) {
+                        defines.set(identifier, value);
+                    }
+
+                    Debug.trace(TRACEID, `${keyword}: [${identifier}] ${value} ${keep ? "" : "IGNORED"}`);
+
+                    // continue on the next line
+                    KEYWORD.lastIndex = define.index + define[0].length;
+                    break;
+                }
+
+                case 'undef': {
+
+                    // read the rest of the define line
+                    UNDEF.lastIndex = match.index;
+                    const undef = UNDEF.exec(source);
+                    const identifier = undef[1].trim();
+
+                    // are we inside if-blocks that are accepted
+                    const keep = Preprocessor._keep(stack);
+
+                    // remove it from defines
+                    if (keep) {
+                        defines.delete(identifier);
+                    }
+
+                    Debug.trace(TRACEID, `${keyword}: [${identifier}] ${keep ? "" : "IGNORED"}`);
+
+                    // continue on the next line
+                    KEYWORD.lastIndex = undef.index + undef[0].length;
+                    break;
+                }
+
+                case 'ifdef':
+                case 'ifndef':
+                case 'if': {
+
+                    // read the if line
+                    IF.lastIndex = match.index;
+                    const iff = IF.exec(source);
+                    const expression = iff[2];
+
+                    // evaluate expression
+                    const evaluated = Preprocessor.evaluate(expression, defines);
+                    error ||= evaluated.error;
+                    let result = evaluated.result;
+                    if (keyword === 'ifndef') {
+                        result = !result;
+                    }
+
+                    // add info to the stack (to be handled later)
+                    stack.push({
+                        anyKeep: result,        // true if any branch was already accepted
+                        keep: result,           // true if this branch is being taken
+                        start: match.index,     // start index if IF line
+                        end: IF.lastIndex       // end index of IF line
+                    });
+
+                    Debug.trace(TRACEID, `${keyword}: [${expression}] => ${result}`);
+
+                    // continue on the next line
+                    KEYWORD.lastIndex = iff.index + iff[0].length;
+                    break;
+                }
+
+                case 'endif':
+                case 'else':
+                case 'elif': {
+
+                    // match the endif
+                    ENDIF.lastIndex = match.index;
+                    const endif = ENDIF.exec(source);
+
+                    const blockInfo = stack.pop();
+
+                    // code between if and endif
+                    const blockCode = blockInfo.keep ? source.substring(blockInfo.end, match.index) : "";
+                    Debug.trace(TRACEID, `${keyword}: [previous block] => ${blockCode !== ""}`);
+
+                    // cut out the IF and ENDIF lines, leave block if required
+                    source = source.substring(0, blockInfo.start) + blockCode + source.substring(ENDIF.lastIndex);
+                    KEYWORD.lastIndex = blockInfo.start + blockCode.length;
+
+                    // handle else if
+                    const endifCommand = endif[1];
+                    if (endifCommand === 'else' || endifCommand === 'elif') {
+
+                        // if any branch was already accepted, all else branches need to fail regardless of the result
+                        let result = false;
+                        if (!blockInfo.anyKeep) {
+                            if (endifCommand === 'else') {
+                                result = !blockInfo.keep;
+                            } else {
+                                const evaluated = Preprocessor.evaluate(endif[2], defines);
+                                result = evaluated.result;
+                                error ||= evaluated.error;
+                            }
+                        }
+
+                        // add back to stack
+                        stack.push({
+                            anyKeep: blockInfo.anyKeep || result,
+                            keep: result,
+                            start: KEYWORD.lastIndex,
+                            end: KEYWORD.lastIndex
+                        });
+                        Debug.trace(TRACEID, `${keyword}: [${endif[2]}] => ${result}`);
+                    }
+
+                    break;
+                }
+            }
+        }
+
+        if (error) {
+            Debug.error("Failed to preprocess shader: ", { source: originalSource });
+            return null;
+        }
+
+        return source;
+    }
+
+    // function returns true if the evaluation is inside keep branches
+    static _keep(stack) {
+        for (let i = 0; i < stack.length; i++) {
+            if (!stack[i].keep)
+                return false;
+        }
+
+        return true;
+    }
+
+    /**
+     * Very simple expression evaluation, handles cases:
+     * expression
+     * defined(expression)
+     * !defined(expression)
+     *
+     * But does not handle more complex cases, which would require more complex system:
+     * defined(A) || defined(B)
+     */
+    static evaluate(expression, defines) {
+
+        const correct = INVALID.exec(expression) === null;
+        Debug.assert(correct, `Resolving expression like this is not supported: ${expression}`);
+
+        // if the format is defined(expression), extract expression
+        let invert = false;
+        const defined = DEFINED.exec(expression);
+        if (defined) {
+            invert = defined[1] === '!';
+            expression = defined[2];
+        }
+
+        // test if expression define exists
+        expression = expression.trim();
+        let exists = defines.has(expression);
+
+        // handle inversion
+        if (invert) {
+            exists = !exists;
+        }
+
+        return {
+            result: exists,
+            error: !correct
+        };
+    }
+}
+
+export { Preprocessor };
diff --git a/src/graphics/program-lib/chunks/lit/clusteredLight.frag.js b/src/graphics/program-lib/chunks/lit/clusteredLight.frag.js
index 599fe2f21..4efe4cab9 100644
--- a/src/graphics/program-lib/chunks/lit/clusteredLight.frag.js
+++ b/src/graphics/program-lib/chunks/lit/clusteredLight.frag.js
@@ -3,6 +3,15 @@ uniform sampler2D clusterWorldTexture;
 uniform sampler2D lightsTexture8;
 uniform highp sampler2D lightsTextureFloat;
 
+// complex ifdef expression are not supported, handle it here
+// defined(CLUSTER_COOKIES) || defined(CLUSTER_SHADOWS)
+#if defined(CLUSTER_COOKIES)
+    #define CLUSTER_COOKIES_OR_SHADOWS
+#endif
+#if defined(CLUSTER_SHADOWS)
+    #define CLUSTER_COOKIES_OR_SHADOWS
+#endif
+
 #ifdef CLUSTER_SHADOWS
     #ifdef GL2
         // TODO: when VSM shadow is supported, it needs to use sampler2D in webgl2
@@ -358,7 +367,7 @@ void evaluateLight(ClusterLightData light) {
             dAtten *= getSpotEffect(light.direction, light.innerConeAngleCos, light.outerConeAngleCos);
         }
 
-        #if defined(CLUSTER_COOKIES) || defined(CLUSTER_SHADOWS)
+        #if defined(CLUSTER_COOKIES_OR_SHADOWS)
 
         if (dAtten > 0.00001) {
 
@@ -441,8 +450,10 @@ void evaluateLight(ClusterLightData light) {
             {
                 vec3 areaDiffuse = (dAttenD * dAtten) * light.color * dAtten3;
 
-                #if defined(CLUSTER_SPECULAR) && defined(CLUSTER_CONSERVE_ENERGY)
-                    areaDiffuse = mix(areaDiffuse, vec3(0), dLTCSpecFres);
+                #if defined(CLUSTER_SPECULAR)
+                    #if defined(CLUSTER_CONSERVE_ENERGY)
+                        areaDiffuse = mix(areaDiffuse, vec3(0), dLTCSpecFres);
+                    #endif
                 #endif
 
                 // area light diffuse - it does not mix diffuse lighting into specular attenuation
@@ -494,9 +505,13 @@ void evaluateLight(ClusterLightData light) {
             {
                 vec3 punctualDiffuse = dAtten * light.color * dAtten3;
 
-                #if defined(CLUSTER_AREALIGHTS) && defined(CLUSTER_SPECULAR) && defined(CLUSTER_CONSERVE_ENERGY)
+                #if defined(CLUSTER_AREALIGHTS)
+                #if defined(CLUSTER_SPECULAR)
+                #if defined(CLUSTER_CONSERVE_ENERGY)
                     punctualDiffuse = mix(punctualDiffuse, vec3(0), dSpecularity);
                 #endif
+                #endif
+                #endif
 
                 dDiffuseLight += punctualDiffuse;
             }
diff --git a/src/graphics/program-lib/chunks/lit/clusteredLightShadows.frag.js b/src/graphics/program-lib/chunks/lit/clusteredLightShadows.frag.js
index 16d2434ac..99a54c14d 100644
--- a/src/graphics/program-lib/chunks/lit/clusteredLightShadows.frag.js
+++ b/src/graphics/program-lib/chunks/lit/clusteredLightShadows.frag.js
@@ -61,7 +61,7 @@ export default /* glsl */`
 
     #endif
 
-    #if defined(CLUSTER_SHADOW_TYPE_PCF3) || defined(CLUSTER_SHADOW_TYPE_PCF5)
+    #if defined(CLUSTER_SHADOW_TYPE_PCF3)
 
     float getShadowOmniClusteredPCF3(sampler2D shadowMap, vec4 shadowParams, vec3 omniAtlasViewport, float shadowEdgePixels, vec3 dir) {
 
@@ -80,7 +80,14 @@ export default /* glsl */`
 
     // we don't have PCF5 implementation for webgl1, use PCF3
     float getShadowOmniClusteredPCF5(sampler2D shadowMap, vec4 shadowParams, vec3 omniAtlasViewport, float shadowEdgePixels, vec3 dir) {
-        return getShadowOmniClusteredPCF3(shadowMap, shadowParams, omniAtlasViewport, shadowEdgePixels, dir);
+
+        float shadowTextureResolution = shadowParams.x;
+        vec2 uv = getCubemapAtlasCoordinates(omniAtlasViewport, shadowEdgePixels, shadowTextureResolution, dir);
+
+        // pcf3
+        float shadowZ = length(dir) * shadowParams.w + shadowParams.z;
+        dShadowCoord = vec3(uv, shadowZ);
+        return getShadowPCF3x3(shadowMap, shadowParams.xyz);
     }
 
     #endif
@@ -129,7 +136,7 @@ export default /* glsl */`
 
     #endif
 
-    #if defined(CLUSTER_SHADOW_TYPE_PCF3) || defined(CLUSTER_SHADOW_TYPE_PCF5)
+    #if defined(CLUSTER_SHADOW_TYPE_PCF3)
 
     float getShadowSpotClusteredPCF3(sampler2D shadowMap, vec4 shadowParams) {
         return getShadowSpotPCF3x3(shadowMap, shadowParams);
@@ -141,7 +148,7 @@ export default /* glsl */`
 
     // we don't have PCF5 implementation for webgl1, use PCF3
     float getShadowSpotClusteredPCF5(sampler2D shadowMap, vec4 shadowParams) {
-        return getShadowSpotClusteredPCF3(shadowMap, shadowParams);
+        return getShadowSpotPCF3x3(shadowMap, shadowParams);
     }
 
     #endif
diff --git a/src/graphics/webgl/webgl-shader.js b/src/graphics/webgl/webgl-shader.js
index f5afc0641..84e136653 100644
--- a/src/graphics/webgl/webgl-shader.js
+++ b/src/graphics/webgl/webgl-shader.js
@@ -1,5 +1,6 @@
 import { Debug } from '../../core/debug.js';
 import { now } from '../../core/time.js';
+import { Preprocessor } from '../../core/preprocessor.js';
 
 import { ShaderInput } from '../shader-input.js';
 import { SHADERTAG_MATERIAL, semanticToLocation } from '../constants.js';
@@ -67,6 +68,10 @@ class WebglShader {
         Debug.assert(definition.vshader, 'No vertex shader has been specified when creating a shader.');
         Debug.assert(definition.fshader, 'No fragment shader has been specified when creating a shader.');
 
+        // resolve ifdefs
+        definition.vshader = Preprocessor.run(definition.vshader);
+        definition.fshader = Preprocessor.run(definition.fshader);
+
         const glVertexShader = this._compileShaderSource(device, definition.vshader, true);
         const glFragmentShader = this._compileShaderSource(device, definition.fshader, false);
 
@@ -298,33 +303,33 @@ class WebglShader {
      * @private
      */
     _processError(src, infoLog) {
-        if (!src)
-            return "";
-
-        const lines = src.split('\n');
         const error = { };
         let code = '';
-        let from = 0;
-        let to = lines.length;
-
-        // if error is in the code, only show nearby lines instead of whole shader code
-        if (infoLog && infoLog.startsWith('ERROR:')) {
-            const match = infoLog.match(/^ERROR:\s([0-9]+):([0-9]+):\s*(.+)/);
-            if (match) {
-                error.message = match[3];
-                error.line = parseInt(match[2], 10);
-
-                from = Math.max(0, error.line - 6);
-                to = Math.min(lines.length, error.line + 5);
+
+        if (src) {
+            const lines = src.split('\n');
+            let from = 0;
+            let to = lines.length;
+
+            // if error is in the code, only show nearby lines instead of whole shader code
+            if (infoLog && infoLog.startsWith('ERROR:')) {
+                const match = infoLog.match(/^ERROR:\s([0-9]+):([0-9]+):\s*(.+)/);
+                if (match) {
+                    error.message = match[3];
+                    error.line = parseInt(match[2], 10);
+
+                    from = Math.max(0, error.line - 6);
+                    to = Math.min(lines.length, error.line + 5);
+                }
             }
-        }
 
-        // Chrome reports shader errors on lines indexed from 1
-        for (let i = from; i < to; i++) {
-            code += (i + 1) + ":\t" + lines[i] + '\n';
-        }
+            // Chrome reports shader errors on lines indexed from 1
+            for (let i = from; i < to; i++) {
+                code += (i + 1) + ":\t" + lines[i] + '\n';
+            }
 
-        error.source = src;
+            error.source = src;
+        }
 
         return [code, error];
     }


Test patch:
diff --git a/test/core/preprocessor.test.mjs b/test/core/preprocessor.test.mjs
new file mode 100644
index 000000000..0299a7206
--- /dev/null
+++ b/test/core/preprocessor.test.mjs
@@ -0,0 +1,133 @@
+import { Preprocessor } from '../../src/core/preprocessor.js';
+
+import { expect } from 'chai';
+
+describe('Preprocessor', function () {
+
+    const srcData = `
+        
+        #define FEATURE1
+        #define FEATURE2
+
+        #ifdef FEATURE1
+            TEST1
+        #endif
+
+        #if defined(FEATURE1)
+            TEST2
+        #endif
+
+        #ifdef FEATURE1
+            #if defined(FEATURE2)
+                TEST3
+            #endif
+        #endif
+
+        #ifndef UNKOWN
+            TEST4
+        #endif
+
+        #if defined (UNKNOWN)
+            TEST5
+        #else
+            TEST6
+        #endif
+
+        $// comment
+
+        // TEST7
+
+        /*
+        TEST8
+        */
+
+        #ifdef UNKNOWN
+            TEST9
+        #elif FEATURE2
+            TEST10
+        #else
+            TEST11
+        #endif
+
+        #undef FEATURE1
+        #ifdef FEATURE1
+            TEST12
+        #endif
+
+        #ifndef FEATURE1
+            TEST13
+        #endif
+
+        #ifdef (UNKNOWN)
+            #define TEST14  // this should not be defined
+        #endif
+    `;
+
+    it('returns false for MORPH_A', function () {
+        expect(Preprocessor.run(srcData).includes('MORPH_A')).to.equal(false);
+    });
+
+    it('returns false for MORPH_B', function () {
+        expect(Preprocessor.run(srcData).includes('MORPH_B')).to.equal(false);
+    });
+
+    it('returns true for $', function () {
+        expect(Preprocessor.run(srcData).includes('$')).to.equal(true);
+    });
+
+    it('returns true for TEST1', function () {
+        expect(Preprocessor.run(srcData).includes('TEST1')).to.equal(true);
+    });
+
+    it('returns true for TEST2', function () {
+        expect(Preprocessor.run(srcData).includes('TEST2')).to.equal(true);
+    });
+
+    it('returns true for TEST3', function () {
+        expect(Preprocessor.run(srcData).includes('TEST3')).to.equal(true);
+    });
+
+    it('returns true for TEST4', function () {
+        expect(Preprocessor.run(srcData).includes('TEST4')).to.equal(true);
+    });
+
+    it('returns false for TEST5', function () {
+        expect(Preprocessor.run(srcData).includes('TEST5')).to.equal(false);
+    });
+
+    it('returns true for TEST6', function () {
+        expect(Preprocessor.run(srcData).includes('TEST6')).to.equal(true);
+    });
+
+    it('returns false for TEST7', function () {
+        expect(Preprocessor.run(srcData).includes('TEST7')).to.equal(false);
+    });
+
+    it('returns false for TEST8', function () {
+        expect(Preprocessor.run(srcData).includes('TEST8')).to.equal(false);
+    });
+
+    it('returns false for TEST9', function () {
+        expect(Preprocessor.run(srcData).includes('TEST9')).to.equal(false);
+    });
+
+    it('returns true for TEST10', function () {
+        expect(Preprocessor.run(srcData).includes('TEST10')).to.equal(true);
+    });
+
+    it('returns false for TEST11', function () {
+        expect(Preprocessor.run(srcData).includes('TEST11')).to.equal(false);
+    });
+
+    it('returns false for TEST12', function () {
+        expect(Preprocessor.run(srcData).includes('TEST12')).to.equal(false);
+    });
+
+    it('returns true for TEST13', function () {
+        expect(Preprocessor.run(srcData).includes('TEST13')).to.equal(true);
+    });
+
+    it('returns false for TEST14', function () {
+        expect(Preprocessor.run(srcData).includes('TEST14')).to.equal(false);
+    });
+});