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.

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

Improve ShaderLab compilation error log and package build #2364

Merged
merged 114 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from 80 commits
Commits
Show all changes
114 commits
Select commit Hold shift + click to select a range
5a26f3d
fix(shader-lab): compatible with empty macro
Sway007 Sep 20, 2023
5ecc318
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Sep 20, 2023
cafc24f
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Sep 20, 2023
dc69489
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Sep 21, 2023
221b7b6
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Sep 21, 2023
0d45d9c
fix(shader-lab): add break and continue syntax
Sway007 Sep 21, 2023
0f14c3f
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Sep 27, 2023
8871d9b
fix: typo
Sway007 Oct 11, 2023
3b4ffd7
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Nov 1, 2023
f649a58
fix(shader-lab): Make usepass compatible with buitin shader
Sway007 Nov 1, 2023
598fc56
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Nov 2, 2023
e33a66f
fix(shader-lab): compatible with no varying variable
Sway007 Nov 2, 2023
41ef06f
feat(shader-lab): detect mismatch return type
Sway007 Nov 6, 2023
b5214fc
fix(shader-lab): renderState assignment
Sway007 Nov 7, 2023
f36ce02
feat: extend material loader data type
Sway007 Nov 13, 2023
ff8b7c2
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Nov 13, 2023
634236f
fix(shader-lab): glsl type pattern
Sway007 Nov 13, 2023
91e6fa4
fix(shader-lab): glsl type pattern
Sway007 Nov 13, 2023
3932448
fix: switch case break
Sway007 Nov 15, 2023
671cace
fix: array index loss
Sway007 Jun 11, 2024
92b972e
feat: merge
Sway007 Jun 11, 2024
9226d38
fix: test-case
Sway007 Jun 11, 2024
ff6a69a
fix: test-case
Sway007 Jun 12, 2024
83b9ca2
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Jun 13, 2024
f510c2a
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Jul 15, 2024
94e30f5
fix: conditional compilation
Sway007 Jul 16, 2024
e3c7aa5
fix: expand token
Sway007 Jul 16, 2024
36b77e0
fix: expand token
Sway007 Jul 16, 2024
b2e07fc
feat: code opt
Sway007 Jul 16, 2024
93df92e
fix: ci
Sway007 Jul 16, 2024
8e0cc28
feat: cache error shader
Sway007 Jul 17, 2024
7652250
Merge branch 'dev/1.3' of https://github.com/galacean/engine into fea…
Sway007 Jul 17, 2024
c92e674
fix: preprocessor parse expression
Sway007 Jul 17, 2024
3fefbe9
Merge branch 'dev/1.3' of https://github.com/galacean/engine into fea…
Sway007 Jul 17, 2024
9eb7398
fix: gles 100 code gen
Sway007 Jul 17, 2024
9f5b5bc
feat: support relative path include
Sway007 Jul 18, 2024
82f73b2
Merge branch 'dev/1.3' of https://github.com/galacean/engine into fea…
Sway007 Jul 18, 2024
964b846
feat: test case
Sway007 Jul 18, 2024
f6fed8b
feat: compatible with builtin shader chunk import
Sway007 Jul 18, 2024
519ab40
feat: code opt
Sway007 Jul 18, 2024
ee6a813
feat: code opt
Sway007 Jul 18, 2024
d7e0ac8
feat: code opt
Sway007 Jul 18, 2024
8f75c28
feat: code opt
Sway007 Jul 18, 2024
00447fb
feat: code opt
Sway007 Jul 18, 2024
d5a3c0c
feat: add log
Sway007 Jul 19, 2024
c176226
feat: add log
Sway007 Jul 19, 2024
b8ca43f
feat: add log
Sway007 Jul 22, 2024
a6d21d6
Merge branch 'dev/1.3' of https://github.com/galacean/engine into fea…
Sway007 Jul 30, 2024
ce23026
fix: multi dist
Sway007 Aug 1, 2024
3bcaeb0
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Aug 5, 2024
79a7a0c
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Aug 5, 2024
fa7131c
Merge branch 'main' of https://github.com/galacean/engine
Sway007 Aug 6, 2024
3093bd2
Merge branch 'main' into feat/shaderlab-error-report
Sway007 Aug 20, 2024
6eba20a
Merge branch 'main' of https://github.com/galacean/engine into feat/s…
Sway007 Aug 20, 2024
8c90796
feat(shaderlab): [wip] optmize error log
Sway007 Aug 23, 2024
dab8003
Merge branch 'main' of https://github.com/galacean/engine into feat/s…
Sway007 Aug 26, 2024
f12b8f7
feat(shaderlab): multi package
Sway007 Aug 27, 2024
c32969b
feat(shaderlab): opt code
Sway007 Aug 27, 2024
9d4d808
feat(shaderlab): opt code
Sway007 Aug 27, 2024
f5edeb3
feat(shaderlab): opt code
Sway007 Aug 27, 2024
68e1556
feat(shaderlab): opt code
Sway007 Aug 27, 2024
dd4777c
feat(shaderlab): test case
Sway007 Aug 27, 2024
1a918da
feat: opt code
Sway007 Aug 27, 2024
4c9e16c
Merge branch 'main' of https://github.com/galacean/engine into feat/s…
Sway007 Aug 27, 2024
e4d267c
feat: opt code
Sway007 Aug 27, 2024
077e009
feat: opt code
Sway007 Aug 27, 2024
462af90
feat: opt code
Sway007 Aug 27, 2024
54f9c79
feat: opt code
Sway007 Aug 27, 2024
d01b060
feat: opt code
Sway007 Aug 27, 2024
df84ce0
feat: opt code
Sway007 Aug 27, 2024
1426499
feat: opt code
Sway007 Aug 27, 2024
48eb236
feat: opt code
Sway007 Aug 27, 2024
3b06a3f
feat: opt code
Sway007 Aug 27, 2024
ab356b6
feat: opt code
Sway007 Aug 27, 2024
ab6de2c
feat: opt code
Sway007 Aug 27, 2024
5092aab
feat: opt code
Sway007 Aug 27, 2024
8a5f838
feat: opt code
Sway007 Aug 27, 2024
ff3f5ae
feat: opt code
Sway007 Aug 27, 2024
c679a9e
feat: opt code
Sway007 Aug 27, 2024
44b7e84
feat: opt code
Sway007 Sep 2, 2024
eb0bcde
feat: code opt
Sway007 Sep 11, 2024
2f56705
feat: code opt
Sway007 Sep 26, 2024
b102ac8
Merge branch 'dev/1.4' of https://github.com/galacean/engine into fea…
Sway007 Sep 26, 2024
e1f749a
feat: add shaderlab verbose package
Sway007 Sep 26, 2024
6e36dd0
feat: add shaderlab verbose package
Sway007 Sep 26, 2024
032a631
feat: update readme
Sway007 Sep 26, 2024
adb871f
feat: rollup.config.js opt
Sway007 Sep 26, 2024
cc8e0a1
feat: code opt & compatible with multiple log
Sway007 Oct 8, 2024
b7da54e
feat: code opt
Sway007 Oct 8, 2024
59776c4
feat: _EDITOR to _VERBOSE
Sway007 Oct 8, 2024
11ebcbc
feat: opt shaderlabobjectpool
Sway007 Oct 8, 2024
67f9c8d
feat: rm redundant code
Sway007 Oct 8, 2024
c76c81f
feat: code opt
Sway007 Oct 9, 2024
2236ac3
feat: code opt
Sway007 Oct 9, 2024
9f8cbe7
feat: code opt
Sway007 Oct 9, 2024
0df3db6
fix: new scope when parse pass
Sway007 Oct 10, 2024
a72aade
feat: code opt
Sway007 Oct 10, 2024
a19e9ba
feat: code opt
Sway007 Oct 11, 2024
4c47c15
feat: code opt
Sway007 Oct 11, 2024
d83cb29
feat: code opt
Sway007 Oct 11, 2024
a180d29
feat: code opt
Sway007 Oct 12, 2024
7c57e15
feat: add testcase for BaseScanner
Sway007 Oct 12, 2024
4c63401
feat: code opt
Sway007 Oct 12, 2024
87c719d
feat: code opt
Sway007 Oct 12, 2024
2661cae
feat: code opt
Sway007 Oct 12, 2024
a85ee45
feat: code opt
Sway007 Oct 12, 2024
4c50206
feat: code opt
Sway007 Oct 12, 2024
cae81c2
feat: code opt
Sway007 Oct 12, 2024
eb515be
feat: code opt
Sway007 Oct 12, 2024
fb3974e
feat: code opt
Sway007 Oct 12, 2024
f0742cf
feat: code opt
Sway007 Oct 12, 2024
ef6496b
feat: code opt
Sway007 Oct 14, 2024
4197f1d
feat: code opt
Sway007 Oct 16, 2024
2e343bb
feat: code opt
Sway007 Oct 16, 2024
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
6 changes: 3 additions & 3 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ jobs:

- name: Install
run: pnpm install
- run: npm run build:editor
- run: npm run build

codecov:
runs-on: macos-latest
Expand All @@ -66,7 +66,7 @@ jobs:
- name: Install
run: pnpm install
- name: Build
run: npm run build:editor
run: npm run build
- name: Test
run: npm run test-cov
- run: pnpm install codecov -w
Expand Down Expand Up @@ -94,7 +94,7 @@ jobs:
- name: Run Cypress Tests
uses: cypress-io/github-action@v5
with:
build: npm run build:editor
build: npm run build
start: npm run e2e:case
wait-on: "http://localhost:5175"
wait-on-timeout: 120
Expand Down
4 changes: 0 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,12 @@
"build:editor": "npm run b:module:editor && npm run b:types",
"lint": "eslint packages/*/src --ext .ts",
"watch": "cross-env NODE_ENV=release BUILD_TYPE=MODULE rollup -cw -m inline",
"watch:editor": "cross-env NODE_ENV=editor BUILD_TYPE=MODULE rollup -cw -m inline",
"watch:umd": "cross-env NODE_ENV=release BUILD_TYPE=UMD rollup -cw -m inline",
"watch:umd:editor": "cross-env NODE_ENV=editor BUILD_TYPE=UMD rollup -cw -m inline",
"b:types": "pnpm -r --filter=./packages/* run b:types",
"b:module": "cross-env BUILD_TYPE=MODULE NODE_ENV=release rollup -c",
"b:module:editor": "cross-env BUILD_TYPE=MODULE NODE_ENV=editor rollup -c",
"b:umd": "cross-env BUILD_TYPE=UMD NODE_ENV=release rollup -c",
"b:miniprogram": "cross-env BUILD_TYPE=MINI rollup -c",
"b:all": "cross-env NODE_ENV=release npm run b:types && cross-env BUILD_TYPE=ALL NODE_ENV=release rollup -c",
"b:all:editor": "cross-env NODE_ENV=editor npm run b:types && cross-env BUILD_TYPE=ALL rollup -c",
"clean": "pnpm -r exec rm -rf dist && pnpm -r exec rm -rf types",
"e2e:case": "pnpm -C ./e2e run case",
"e2e": "cypress run --browser chrome --headless",
Expand Down
13 changes: 13 additions & 0 deletions packages/shader-lab/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,19 @@ const shader = Shader.create(galaceanShaderCode);
engine.run()
```

There are two versions of ShaderLab: `Release` and `Debug`. The Debug version offers more user-friendly diagnostic information for debug ShaderLab compilation errors, while the Release version provides superior performance.

you can use debug version by import:

```ts
// umd
import { ShaderLab } from "@galacean/engine-shader-lab/dist/browser.editor";
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved
// esmoudle
import { ShaderLab } from "@galacean/engine-shader-lab/dist/module.editor";
// commonjs
import { ShaderLab } from "@galacean/engine-shader-lab/dist/main.editor";
```

GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved
## CFG Grammar conflict detection

The Galacean ShaderLab syntax is defined using Context-Free Grammar (CFG) and is documented within the `\*.y` file. When modifications to the ShaderLab syntax are required, it is recommended to make changes to the existing CFG syntax file, and employ [Bison](https://www.gnu.org/software/bison/manual/bison.html) to detect any potential grammar conflicts.
Expand Down
72 changes: 65 additions & 7 deletions packages/shader-lab/src/Error.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,77 @@
// #if _EDITOR
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved
import { ShaderRange } from "./common";
import { Logger } from "@galacean/engine";
import { ShaderPosition, ShaderRange } from "./common";
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved

export abstract class GSError extends Error {
readonly loc: ShaderRange;
static wrappingLineCount = 2;

constructor(message: string, loc: ShaderRange, cause?: Error) {
readonly loc: ShaderRange | ShaderPosition;
readonly source: string;
readonly file?: string;

constructor(message: string, loc: ShaderRange | ShaderPosition, source: string, file?: string, cause?: Error) {
super(message, { cause });
this.loc = loc;
this.source = source;
this.file = file;
}

log(_source?: string): void {
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved
if (!Logger.enable) return;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure toString method always returns a string

The toString method may return undefined if Logger.enable is false. Since toString should consistently return a string, consider returning an empty string in this case.

Apply this diff to fix the issue:

-    if (!Logger.enable) return;
+    if (!Logger.enable) return '';
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!Logger.enable) return;
if (!Logger.enable) return '';

let start: ShaderPosition, end: ShaderPosition;
const { message, loc, source: originSource } = this;
let source = originSource;
if (_source) source = _source;
if (!source) {
Logger.error(message);
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved
return;
}

if (loc instanceof ShaderPosition) {
start = end = loc;
} else {
start = loc.start;
end = loc.end;
}
Sway007 marked this conversation as resolved.
Show resolved Hide resolved
const lines = source.split("\n");

let diagnosticMessage = `${this.name}: ${message}\n\n`;
const lineSplit = "|···";

for (let i = 0; i <= end.line + GSError.wrappingLineCount; i++) {
if (i < start.line - GSError.wrappingLineCount) continue;

diagnosticMessage += lineSplit;
diagnosticMessage += `${lines[i]}\n`;
if (i >= start.line && i <= end.line) {
diagnosticMessage += " ".repeat(lineSplit.length + start.column);
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved
diagnosticMessage += "^".repeat(Math.max(end.column - start.column, 1));
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved
diagnosticMessage += "\n";
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved
}
}

Logger.error(diagnosticMessage);
}
}

export class PreprocessorError extends GSError {
constructor(message: string, loc: ShaderRange | ShaderPosition, source: string, file?: string, cause?: Error) {
super(message, loc, source, file, cause);
this.name = "PreprocessorError";
}
}
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved

export class CompilationError extends GSError {
constructor(message: string, loc: ShaderRange | ShaderPosition, source: string, file?: string, cause?: Error) {
super(message, loc, source, file, cause);
this.name = "CompilationError";
}
}

export class SemanticError extends GSError {
constructor(message: string, loc: ShaderRange, cause?: Error) {
super(message, loc, cause);
this.name = "SemanticError";
export class ScannerError extends GSError {
constructor(message: string, loc: ShaderRange | ShaderPosition, source: string, file?: string, cause?: Error) {
super(message, loc, source, file, cause);
this.name = "ScannerError";
}
}
// #endif
106 changes: 73 additions & 33 deletions packages/shader-lab/src/ShaderLab.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,42 +2,30 @@ import { Lexer } from "./lexer";
import { ShaderTargetParser } from "./parser";
import { Preprocessor } from "./preprocessor";
import { GLES100Visitor, GLES300Visitor } from "./codeGen";
import { IShaderContent, IShaderLab } from "@galacean/engine-design/src/shader-lab";
import { IShaderContent, IShaderLab } from "@galacean/engine-design";
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved
import { ShaderContentParser } from "./contentParser";
// @ts-ignore
import { Logger, ShaderLib, ShaderMacro, ShaderPass, ShaderPlatformTarget } from "@galacean/engine";
import { ShaderPosition, ShaderRange } from "./common";
import { ShaderLabObjectPool } from "./ShaderLabObjectPool";
// #if _EDITOR
import { GSError } from "./Error";
// #endif
import { PpParser } from "./preprocessor/PpParser";

export class ShaderLab implements IShaderLab {
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved
/**
* @internal
*/
private static _parser = ShaderTargetParser.create();
/**
* @internal
*/
private static _shaderPositionPool = new ShaderLabObjectPool(ShaderPosition);
private static _shaderRangePool = new ShaderLabObjectPool(ShaderRange);
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved

/**
* @internal
*/
private static _shaderRangePool = new ShaderLabObjectPool(ShaderRange);
static _processingPassText?: string;
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved

static createPosition(
index: number,
// #if _EDITOR
line?: number,
column?: number
// #endif
): ShaderPosition {
static createPosition(index: number, line?: number, column?: number): ShaderPosition {
const position = this._shaderPositionPool.get();
position.setX(
index,
// #if _EDITOR
line,
column
// #endif
);
position.setX(index, line, column);
Copy link

Choose a reason for hiding this comment

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

Refactor suggestion: Use class name instead of this in static context.

Using this in a static context can be confusing. Use the class name instead.

Apply this diff to refactor the code:

- const position = this._shaderPositionPool.get();
+ const position = ShaderLab._shaderPositionPool.get();
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
static createPosition(index: number, line?: number, column?: number): ShaderPosition {
const position = this._shaderPositionPool.get();
position.setX(
index,
// #if _EDITOR
line,
column
// #endif
);
position.setX(index, line, column);
static createPosition(index: number, line?: number, column?: number): ShaderPosition {
const position = ShaderLab._shaderPositionPool.get();
position.setX(index, line, column);
Tools
Biome

[error] 27-27: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved
return position;
}

Expand All @@ -47,6 +35,17 @@ export class ShaderLab implements IShaderLab {
return range;
}

// #if _EDITOR
private _errors: GSError[] = [];
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved

/**
* Retrieve the compilation errors
*/
get errors(): GSError[] | undefined {
Copy link
Member

Choose a reason for hiding this comment

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

不是最多空数组么,怎么会有 undefined

Copy link
Member Author

Choose a reason for hiding this comment

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

done

return this._errors;
}
// #endif

_parseShaderPass(
source: string,
vertexEntry: string,
Expand All @@ -56,7 +55,6 @@ export class ShaderLab implements IShaderLab {
platformMacros: string[],
basePathForIncludeKey: string
) {
ShaderLabObjectPool.clearAllShaderLabObjectPool();
Preprocessor.reset(ShaderLib, basePathForIncludeKey);
for (const macro of macros) {
Preprocessor.addPredefinedMacro(macro.name, macro.value);
Expand All @@ -66,36 +64,67 @@ export class ShaderLab implements IShaderLab {
Preprocessor.addPredefinedMacro(platformMacros[i]);
}

// #if _EDITOR
// TODO: index to position
// Logger.convertSourceIndex = Preprocessor.convertSourceIndex.bind(Preprocessor);
// #endif

const preprocessorStart = performance.now();

const ppdContent = Preprocessor.process(source);
// #if _EDITOR
if (PpParser._errors.length > 0) {
for (const err of PpParser._errors) {
this._errors.push(<GSError>err);
}
this._logErrors(this._errors);
return { vertex: "", fragment: "" };
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider adding tests to cover error handling in the preprocessor step

To ensure the robustness of the error handling logic introduced in the preprocessor step, please add unit tests that cover scenarios where PpParser._errors contains errors. This will help validate that errors are correctly accumulated and that the method _logErrors behaves as expected.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 68-69: packages/shader-lab/src/ShaderLab.ts#L68-L69
Added lines #L68 - L69 were not covered by tests


[warning] 71-72: packages/shader-lab/src/ShaderLab.ts#L71-L72
Added lines #L71 - L72 were not covered by tests

}
// #endif

Logger.info(`[pass compilation - preprocessor] cost time ${performance.now() - preprocessorStart}ms`);

const lexer = new Lexer(ppdContent);
const tokens = lexer.tokenize();
const program = ShaderLab._parser.parse(tokens);

const { _parser: parser } = ShaderLab;

ShaderLab._processingPassText = ppdContent;
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved
const program = parser.parse(tokens);

// #if _EDITOR
for (const err of parser.errors) {
this._errors.push(err);
}
if (!program) {
this._logErrors(this._errors);
return { vertex: "", fragment: "" };
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add unit tests for error handling in the parser stage

The error handling code after parsing tokens is critical for capturing and reporting parsing errors. Consider adding unit tests to cover cases where parser.errors contains errors and when program is undefined. This will ensure that the error accumulation and logging mechanisms function correctly.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 91-91: packages/shader-lab/src/ShaderLab.ts#L91
Added line #L91 was not covered by tests

// #endif

const codeGen =
backend === ShaderPlatformTarget.GLES100 ? GLES100Visitor.getVisitor() : GLES300Visitor.getVisitor();

const start = performance.now();
const ret = codeGen.visitShaderProgram(program, vertexEntry, fragmentEntry);
Logger.info(`[CodeGen] cost time: ${performance.now() - start}ms`);
ShaderLab._processingPassText = undefined;

// #if _EDITOR
for (const err of codeGen.errors) {
this._errors.push(err);
}
this._logErrors(this._errors);
// #endif
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure code generation error handling is tested

The error handling added during the code generation phase is important for diagnosing code generation issues. Please add unit tests that cover situations where codeGen.errors has entries. This will help verify that errors are properly collected in this._errors and that _logErrors effectively logs these errors.


return ret;
}

_parseShaderContent(shaderSource: string): IShaderContent {
ShaderLabObjectPool.clearAllShaderLabObjectPool();
ShaderContentParser.reset();
return ShaderContentParser.parse(shaderSource);
// #if _EDITOR
this._errors.length = 0;
// #endif
const ret = ShaderContentParser.parse(shaderSource);
for (const error of ShaderContentParser._errors) {
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved
this._errors.push(error);
}
return ret;
}

// #if _EDITOR
Expand All @@ -105,8 +134,8 @@ export class ShaderLab implements IShaderLab {
*/
_parse(
shaderSource: string,
macros: ShaderMacro[],
backend: ShaderPlatformTarget
macros: ShaderMacro[] = [],
backend: ShaderPlatformTarget = ShaderPlatformTarget.GLES100
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved
): (ReturnType<ShaderLab["_parseShaderPass"]> & { name: string })[] {
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved
const structInfo = this._parseShaderContent(shaderSource);
const passResult = [] as any;
Expand All @@ -129,5 +158,16 @@ export class ShaderLab implements IShaderLab {
}
return passResult;
}

/**
* @internal
*/
_logErrors(errors: GSError[], source?: string) {
if (errors.length === 0 || !Logger.isEnabled) return;
Logger.error(`${errors.length} errors occur!`);
for (const err of errors) {
err.log(source);
}
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Typographical error in log message and recommendation for tests.

  1. Typo in log message:

    • The log message in line 167 should use past tense for correct grammar.

    Apply this diff to fix the typo:

    - Logger.error(`${errors.length} errors occur!`);
    + Logger.error(`${errors.length} errors occurred!`);
  2. Consider adding tests for _logErrors method:

    • To ensure that error logging behaves as expected under various conditions, consider adding unit tests for _logErrors.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
_logErrors(errors: GSError[], source?: string) {
if (errors.length === 0 || !Logger.isEnabled) return;
Logger.error(`${errors.length} errors occur!`);
for (const err of errors) {
err.log(source);
}
}
_logErrors(errors: GSError[], source?: string) {
if (errors.length === 0 || !Logger.isEnabled) return;
Logger.error(`${errors.length} errors occurred!`);
for (const err of errors) {
err.log(source);
}
}
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 167-169: packages/shader-lab/src/ShaderLab.ts#L167-L169
Added lines #L167 - L169 were not covered by tests

// #endif
}
8 changes: 1 addition & 7 deletions packages/shader-lab/src/Utils.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
import { ENonTerminal, GrammarSymbol } from "./parser/GrammarSymbol";
import { BaseToken as Token } from "./common/BaseToken";
import { EKeyword, ETokenType, GalaceanDataType, ShaderRange, ShaderPosition } from "./common";
import { EKeyword, ETokenType, GalaceanDataType } from "./common";
import { TreeNode } from "./parser/AST";
// #if _EDITOR
import State from "./lalr/State";
// #endif
import { Logger } from "@galacean/engine";

export class ParserUtils {
static unwrapNodeByType<T = TreeNode>(node: TreeNode, type: ENonTerminal): T | undefined {
Expand Down Expand Up @@ -39,11 +38,6 @@ export class ParserUtils {
return sm < ENonTerminal.START;
}

static throw(pos: ShaderPosition | ShaderRange | number, ...msgs: any[]) {
Logger.error(pos.toString(), ...msgs);
throw msgs.join(" ");
}

/**
* @internal
*/
Expand Down
Loading
Loading