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

Fix minor shader compiler issues #12369

Merged
merged 1 commit into from
Apr 11, 2022

Conversation

kaliatech
Copy link
Contributor

@kaliatech kaliatech commented Apr 9, 2022

Changes:

  • Allow semicolons in shader single line comments
  • Allow indented multiline macros

Related post in forum showing reproduction for the single line comment issue:

There are no reports for the indented multiline macro issue, but I verified locally that indenting a multiline macro would break compiler, and with this fix, it works.

@azure-pipelines
Copy link

Please make sure to tag your PR with "bug", "new feature" or "breaking change" tags.

@azure-pipelines
Copy link

Snapshot stored with reference name:
refs/pull/12369/merge

Test environment:
https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12369/merge/index.html

To test a playground add it to the URL, for example:

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12369/merge/index.html#WGZLGJ#4600

To test the snapshot in the playground itself use (for example):

https://playground.babylonjs.com/?snapshot=refs/pull/12369/merge#BCU1XR#0

Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

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

Nice :-)

@sebavan sebavan requested a review from deltakosh April 9, 2022 15:23
@azure-pipelines
Copy link

Visualization tests for webgl1 have failed. If some tests failed because the snapshots do not match, the report can be found at

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12369/merge/testResults/webgl1/index.html

If tests were successful afterwards, this report might not be available anymore.

@sebavan sebavan self-requested a review April 9, 2022 15:47
@azure-pipelines
Copy link

Visualization tests for webgl2 have failed. If some tests failed because the snapshots do not match, the report can be found at

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12369/merge/testResults/webgl2/index.html

If tests were successful afterwards, this report might not be available anymore.

@sebavan
Copy link
Member

sebavan commented Apr 9, 2022

This loooks like a great start but could you merge back master in so that we keep only the issues related to the PBR ? I am seeing some iridescence issues related to master in the build.

@kaliatech kaliatech force-pushed the bug-shader-comments-indents branch from 2a83411 to 570f447 Compare April 9, 2022 20:28
@azure-pipelines
Copy link

Please make sure to tag your PR with "bug", "new feature" or "breaking change" tags.

@azure-pipelines
Copy link

Snapshot stored with reference name:
refs/pull/12369/merge

Test environment:
https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12369/merge/index.html

To test a playground add it to the URL, for example:

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12369/merge/index.html#WGZLGJ#4600

To test the snapshot in the playground itself use (for example):

https://playground.babylonjs.com/?snapshot=refs/pull/12369/merge#BCU1XR#0

@azure-pipelines
Copy link

Visualization tests for webgl1 have failed. If some tests failed because the snapshots do not match, the report can be found at

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12369/merge/testResults/webgl1/index.html

If tests were successful afterwards, this report might not be available anymore.

@azure-pipelines
Copy link

Visualization tests for webgl2 have failed. If some tests failed because the snapshots do not match, the report can be found at

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12369/merge/testResults/webgl2/index.html

If tests were successful afterwards, this report might not be available anymore.

@kaliatech
Copy link
Contributor Author

kaliatech commented Apr 9, 2022

could you merge back master in so that we keep only the issues related to the PBR

I rebased back to master before your recent PR. The tests are still failing and it does seem to be due to changes I made in this PR. Looking at some of the failing test PGs, the underlying reason is not obvious to me though. My guess is that's due to my 2nd commit to allow for multiline macros, but that's just a guess.

If not obvious to anyone else, I will try running/debugging the failing tests next week.

@BlakeOne
Copy link
Contributor

BlakeOne commented Apr 10, 2022

Hey @kaliatech, it seems pushing the trimmed line for the macro line instead of the original line fixes the 3 PG's that were failing validation, although I'm not sure why would be necessary... Like below for example. :)

const trimmedLine = line.trim();
if (trimmedLine[0] === "#") {
    this._lines.push(trimmedLine);
    continue;
}

@kaliatech
Copy link
Contributor Author

Handling multiline macros got complicated with a few different issues. I removed that commit and so this PR now only addresses the semicolons in the single line comment issue. I'll add more info to the forum post.

@azure-pipelines
Copy link

Please make sure to tag your PR with "bug", "new feature" or "breaking change" tags.

@azure-pipelines
Copy link

Snapshot stored with reference name:
refs/pull/12369/merge

Test environment:
https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12369/merge/index.html

To test a playground add it to the URL, for example:

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12369/merge/index.html#WGZLGJ#4600

To test the snapshot in the playground itself use (for example):

https://playground.babylonjs.com/?snapshot=refs/pull/12369/merge#BCU1XR#0

@RaananW RaananW added the bug label Apr 11, 2022
@RaananW RaananW merged commit 055602d into BabylonJS:master Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants