Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix generic type deduce bug of ShaderLab compiler in verbose mode #2477

Merged
merged 5 commits into from
Jan 9, 2025
Merged

Fix generic type deduce bug of ShaderLab compiler in verbose mode #2477

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e348595
feat: disable glsl compress for non mini package
Sway007 Dec 18, 2024
6c99773
Merge branch 'dev/1.4' of github.com:galacean/engine into dev/1.4
Sway007 Dec 25, 2024
15eb742
fix: generic function type deduce
Sway007 Dec 25, 2024
1171cf8
Merge branch 'dev/1.4' of github.com:galacean/engine into fix/builtin…
Sway007 Jan 8, 2025
1f2414a
feat: remove redundent code
Sway007 Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/shader-lab/src/parser/AST.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,9 +717,9 @@ export namespace ASTNode {
}
}
// #if _VERBOSE
const builtinFn = BuiltinFunction.getFn(fnIdent, ...(paramSig ?? []));
const builtinFn = BuiltinFunction.getFn(fnIdent, paramSig);
if (builtinFn) {
this.type = BuiltinFunction.getReturnType(builtinFn.fun, builtinFn.genType);
this.type = builtinFn.realReturnType;
return;
}
// #endif
Expand Down
59 changes: 34 additions & 25 deletions packages/shader-lab/src/parser/builtin/functions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ export class BuiltinFunction {
ident: string;
readonly args: BuiltinType[];
readonly scope: EShaderStage;
signatures: NonGenericGalaceanType[] = [];

get realReturnType(): NonGenericGalaceanType {
return this.signatures[0];
}
Sway007 marked this conversation as resolved.
Show resolved Hide resolved

private constructor(ident: string, returnType: BuiltinType, scope: EShaderStage, ...args: BuiltinType[]) {
this.ident = ident;
Expand Down Expand Up @@ -59,33 +64,37 @@ export class BuiltinFunction {
BuiltinFunctionTable.set(ident, list);
}

static getFn(
ident: string,
...args: BuiltinType[]
): { fun: BuiltinFunction; genType: Exclude<GalaceanDataType, string> } | undefined {
static getFn(ident: string, parameterTypes: NonGenericGalaceanType[]): BuiltinFunction | undefined {
const list = BuiltinFunctionTable.get(ident);
let realType = TypeAny;
if (list?.length) {
const fun = list.find((item) => {
if (item.args.length !== args.length) return false;
let genType = 0;
for (let i = 0; i < args.length; i++) {
if (args[i] === TypeAny) continue;
realType = args[i];
if (isGenericType(item.args[i])) {
if (genType === 0) {
genType = args[i];
continue;
} else {
realType = genType;
if (list) {
for (let length = list.length, i = 0; i < length; i++) {
const fn = list[i];
const fnArgs = fn.args;
const argLength = fnArgs.length;
if (argLength !== parameterTypes.length) continue;
// Try to match generic parameter type.
let returnType = TypeAny;
let found = true;
for (let i = 0; i < argLength; i++) {
const curFnArg = fnArgs[i];
if (isGenericType(curFnArg)) {
if (returnType === TypeAny) returnType = parameterTypes[i];
Sway007 marked this conversation as resolved.
Show resolved Hide resolved
} else {
if (curFnArg !== parameterTypes[i] && parameterTypes[i] !== TypeAny) {
found = false;
break;
Sway007 marked this conversation as resolved.
Show resolved Hide resolved
}
}
if (args[i] === TypeAny) continue;
if (args[i] !== realType) return false;
}
return true;
});
if (fun) return { fun, genType: realType };
if (found) {
fn.signatures.length = 0;
fn.signatures.push(returnType);
zhuxudong marked this conversation as resolved.
Show resolved Hide resolved
for (let i = 0; i < argLength; i++) {
fn.signatures.push(parameterTypes[i]);
zhuxudong marked this conversation as resolved.
Show resolved Hide resolved
}
return fn;
}
}
}
}
}
Expand Down Expand Up @@ -249,8 +258,8 @@ BuiltinFunction._create("textureSize", EKeyword.IVEC2, EKeyword.SAMPLER_CUBE_SHA
BuiltinFunction._create("textureSize", EKeyword.IVEC3, EGenType.GSampler2DArray, EKeyword.INT);
BuiltinFunction._create("textureSize", EKeyword.IVEC3, EKeyword.SAMPLER2D_ARRAY_SHADOW, EKeyword.INT);

BuiltinFunction._create("texture2D", EKeyword.SAMPLER2D, EKeyword.VEC2);
BuiltinFunction._create("texture2D", EKeyword.SAMPLER2D, EKeyword.VEC2, EKeyword.FLOAT);
BuiltinFunction._create("texture2D", EKeyword.VEC4, EKeyword.SAMPLER2D, EKeyword.VEC2);
BuiltinFunction._create("texture2D", EKeyword.VEC4, EKeyword.SAMPLER2D, EKeyword.VEC2, EKeyword.FLOAT);

BuiltinFunction._create("texture", EGenType.GVec4, EGenType.GSampler2D, EKeyword.VEC2, EKeyword.FLOAT);
BuiltinFunction._create("texture", EGenType.GVec4, EGenType.GSampler2D, EKeyword.VEC2);
Expand Down
3 changes: 3 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion tests/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
"@galacean/engine-rhi-webgl": "workspace:*",
"@galacean/engine-physics-lite": "workspace:*",
"@galacean/engine-shaderlab": "workspace:*",
"@galacean/engine-physics-physx": "workspace:*"
"@galacean/engine-physics-physx": "workspace:*",
"@galacean/engine-shader-shaderlab": "workspace:*"
},
"devDependencies": {
"@vitest/browser": "2.1.3"
Expand Down
30 changes: 30 additions & 0 deletions tests/src/shader-lab/ShaderLab.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { Color } from "@galacean/engine-math";
import { ShaderLab as ShaderLabVerbose, GSError } from "@galacean/engine-shaderlab/verbose";
import { ShaderLab as ShaderLabRelease } from "@galacean/engine-shaderlab";
import { glslValidate, shaderParse } from "./ShaderValidate";
import { registerIncludes } from "@galacean/engine-shader-shaderlab";

import { IShaderContent } from "@galacean/engine-design";
import { describe, beforeAll, expect, assert, it } from "vitest";
Expand All @@ -17,6 +18,28 @@ const { readFile } = server.commands;

const demoShader = await readFile("./shaders/demo.shader");

const commonMacros = [
{ name: "RENDERER_IS_RECEIVE_SHADOWS" },
{ name: "MATERIAL_IS_TRANSPARENT" },
{ name: "RENDERER_HAS_UV" },
{ name: "RENDERER_HAS_NORMAL" },
{ name: "RENDERER_HAS_TANGENT" },
{ name: "SCENE_FOG_MODE", value: "0" },
{ name: "SCENE_SHADOW_CASCADED_COUNT", value: "1" },
{ name: "CAMERA_ORTHOGRAPHIC" },
{ name: "MATERIAL_NEED_WORLD_POS" },
{ name: "MATERIAL_NEED_TILING_OFFSET" },
{ name: "SCENE_DIRECT_LIGHT_COUNT", value: "1" },
{ name: "MATERIAL_ENABLE_SS_REFRACTION" },
{ name: "MATERIAL_HAS_TRANSMISSION" },
{ name: "MATERIAL_HAS_THICKNESS" },
{ name: "MATERIAL_HAS_ABSORPTION" },
{ name: "MATERIAL_HAS_TRANSMISSION_TEXTURE" },
{ name: "REFRACTION_SPHERE" }
]
.map((item) => `#define ${item.name} ${item.value ?? ""}`)
.join("\n");

function toString(v: Color): string {
return `Color(${v.r}, ${v.g}, ${v.b}, ${v.a})`;
}
Expand Down Expand Up @@ -124,6 +147,13 @@ describe("ShaderLab", () => {
expect(passList[0].isUsePass).to.be.true;
expect(passList[0].name).eq("pbr/Default/Forward");
pass1 = passList[1];
registerIncludes();
});

it("builtin-function", async () => {
let shaderSource = await readFile("./shaders/builtin-function.shader");
shaderSource = shaderSource.replace("__$$insert_maros$$__", commonMacros);
glslValidate(shaderSource, shaderLabVerbose, {});
Comment on lines +153 to +156
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Review comment is accurate - test needs improvement

The test case doesn't properly validate the generic type deduction bug fix. The shader file doesn't contain any specific tests for the clamp function with vector types, and the test only verifies basic shader compilation. The original review suggestions remain valid:

  1. Add explicit test cases using clamp with vec3 parameters
  2. Include assertions to verify correct type inference
  3. Add error cases with type mismatches
🔗 Analysis chain

Enhance test coverage for the generic type deduction bug.

The current test case doesn't fully validate the fix for the generic type deduction bug. Consider:

  1. Adding specific assertions to verify the correct type inference for the clamp function
  2. Including test cases for both correct and incorrect generic type deductions
  3. Adding error cases to ensure the compiler properly handles type mismatches

Run this script to check if the test shader contains the relevant clamp function usage:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify if the test shader contains the clamp function usage with vec3 type
# Expected: Find usage of clamp with vec3 parameter

rg -A 3 'clamp\s*\(\s*vec3' ./shaders/builtin-function.shader

Length of output: 137

Script:

#!/bin/bash
# Find all shader files in the repository
fd -e shader

# If no .shader files found, try looking for common shader extensions
fd -e glsl -e vert -e frag

Length of output: 7753

Script:

#!/bin/bash
# Check the content of builtin-function shader
cat tests/src/shader-lab/shaders/builtin-function.shader

# Also search for any clamp usage in other shader files for context
rg "clamp\s*\(" --type glsl -A 2

Length of output: 1046

Script:

#!/bin/bash
# Search for clamp usage in all files without type restriction
rg "clamp\s*\(" -A 2

# Check the content of ForwardPassPBR.glsl
cat packages/shader-shaderlab/src/shaders/shadingPBR/ForwardPassPBR.glsl

Length of output: 27020

});

it("create shaderLab", async () => {
Expand Down
5 changes: 5 additions & 0 deletions tests/src/shader-lab/ShaderValidate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,11 @@ export function glslValidate(shaderSource, _shaderLab?: ShaderLab, includeMap =
// @ts-ignore
ShaderPass._shaderRootPath
);
if (shaderLab.errors) {
for (const error of shaderLab.errors) {
console.error(error.toString());
}
}
Sway007 marked this conversation as resolved.
Show resolved Hide resolved
validateShaderPass(pass, compiledPass.vertex, compiledPass.fragment);
});
});
Expand Down
37 changes: 37 additions & 0 deletions tests/src/shader-lab/shaders/builtin-function.shader
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
Shader "/Folder1/test.gs" {
SubShader "Default" {
UsePass "pbr/Default/ShadowCaster"

Pass "Forward Pass" {
Tags { pipelineStage = "Forward"}

DepthState {
WriteEnabled = depthWriteEnabled;
}

BlendState {
Enabled = blendEnabled;
SourceColorBlendFactor = sourceColorBlendFactor;
DestinationColorBlendFactor = destinationColorBlendFactor;
SourceAlphaBlendFactor = sourceAlphaBlendFactor;
DestinationAlphaBlendFactor = destinationAlphaBlendFactor;
}

RasterState{
CullMode = rasterStateCullMode;
}

RenderQueueType = renderQueueType;

#define IS_METALLIC_WORKFLOW
#define MATERIAL_ENABLE_IRIDESCENCE

__$$insert_maros$$__

VertexShader = PBRVertex;
FragmentShader = PBRFragment;

#include "ForwardPassPBR.glsl"
}
}
}
Loading