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

Handle case insensitive FXC HLSL keywords. #2347

Merged
merged 3 commits into from
May 31, 2023

Conversation

PJB3005
Copy link
Contributor

@PJB3005 PJB3005 commented May 18, 2023

Honestly I realized while almost done with this that maybe trying to put maintenance effort into FXC rather than DXC wouldn't be worth it, but OH WELL. Feel free to deny this PR if we don't want to add bugfixes for FXC.

There are a few keywords like "pass" in HLSL that are actually case-insensitive for FXC. This can be disabled with strict mode, but even if you do that FXC will continue to give an error if you try to use them in identifiers (at all, with any casing).

This changes the namer code to escape these keywords even if the casing is different.

If you're wondering where I got the list from: I looked at the list of strings in D3DCompiler_47.dll. None of this is documented.

@PJB3005 PJB3005 force-pushed the 23-05-18-case-insensitive-keyword branch 2 times, most recently from b51a3b9 to 4e6480d Compare May 18, 2023 15:11
There are a few keywords like "pass" in HLSL that are actually case-insensitive for FXC. This can be disabled with strict mode, but even if you do that FXC will continue to give an error if you try to use them in identifiers (at all, with any casing).

This changes the namer code to escape these keywords even if the casing is different.

If you're wondering where I got the list from: I looked at the list of strings in D3DCompiler_47.dll.
@PJB3005 PJB3005 force-pushed the 23-05-18-case-insensitive-keyword branch from 4e6480d to 30471e4 Compare May 18, 2023 15:28
@jimblandy
Copy link
Member

We do need FXC support for the time being.

@jimblandy jimblandy added kind: bug Something isn't working area: back-end Outputs of shader conversion lang: HLSL High-Level Shading Language labels May 22, 2023
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I double checked FXC's behavior and this seems needed for the time being.

src/back/hlsl/keywords.rs Outdated Show resolved Hide resolved
src/proc/namer.rs Outdated Show resolved Hide resolved
Remove dependency on unicase

Remove duplicate TextureCube keyword
@PJB3005 PJB3005 force-pushed the 23-05-18-case-insensitive-keyword branch from 8935654 to 8265509 Compare May 26, 2023 20:29
@PJB3005 PJB3005 requested a review from teoxoy May 26, 2023 20:30
src/proc/namer.rs Outdated Show resolved Hide resolved
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@teoxoy teoxoy merged commit 544ccf8 into gfx-rs:master May 31, 2023
@PJB3005 PJB3005 deleted the 23-05-18-case-insensitive-keyword branch May 31, 2023 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: back-end Outputs of shader conversion kind: bug Something isn't working lang: HLSL High-Level Shading Language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants