Skip to content

fix: SG-43607: Bugfix/aces2 support#1288

Open
tjjackson wants to merge 1 commit into
AcademySoftwareFoundation:mainfrom
dreamworksanimation:bugfix/aces2_support
Open

fix: SG-43607: Bugfix/aces2 support#1288
tjjackson wants to merge 1 commit into
AcademySoftwareFoundation:mainfrom
dreamworksanimation:bugfix/aces2_support

Conversation

@tjjackson

Copy link
Copy Markdown
Contributor

Linked issues

#1275

Summarize your change.

Added a check to functionsMissingLutAsParameter() to avoid treating "else if" as a function definition.

Describe the reason for the change.

The logic added in my previous PR uses a regex to search for the start of functions. The regex roughly follows the format of "word word (...) {".

"else if (...) {" matches as a false positive, so if there is an "else if" block that references a LUT, the code will treat all insteads of "if (...)" as a function call that needs the LUT added as a parameter, and breaks the code.

To the best of my knowledge, "else if" may be the only combination of GLSL keywords that could match the regex as a false positive, so I just added logic to check if the first word matched is "else".

Describe what you have tested and on which operating system.

Tested it on Rocky Linux 9.7

Signed-off-by: TJ Jackson <tj.jackson@dreamworks.com>
searchBegin = match[0].second;
continue;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

its not just else, in my case it was also happening on if. I think we should check all glsl keywords perhaps something like this

Suggested change
static const std::unordered_set<std::string> glslKeywords = {
"if", "else", "for", "while", "do", "switch", "case",
"return", "break", "continue", "discard", "struct"
};
while (std::regex_search(searchBegin, inout_glsl.cend(), match, functionStartRegex))
{
// The regex can match "else if (...) {" as returnType=else,
// functionName=if. Skip any match where the name is a
// GLSL keyword.
if (glslKeywords.count(match.str(2)))
{
searchBegin = match[0].second;
continue;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In the example GLSL you provided (thank you for that), functionsMissingLutAsParameter() is matching to the "else if (...) {" statements on line 98, and then returns "if" as a function that needs to have lut parameters passed to it. shaderAddLutAsParameter() then goes through and finds every instance of "if (,,,)" and inserts the LUT. An if statement on its own would not have triggered this behavior, as it would not have matched the functionStartRegex. As far as I'm aware, "else if" is the only two-word combination of GLSL keywords that could precede "(...) {", so that's why I limited my check to just that.

I'm fine adding checks for other GLSL keywords, if people would prefer that, it just may not be technically necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @tjjackson and @markreidvfx , thanks for fixing this ! I have repro steps with a standard config to reproduce this issue and we would very much like to include this fix for the upcoming RV 2026/Open RV 4.0 release. Both the "else" guard and the keyword-set guard fix it, but I'd go with Mark's suggestion if you don't mind as it is more robust and self-documenting since a real function name can never be a reserved word. So we could add it to the comment maybe:

Suggested change
// A real function's name can never be a GLSL reserved keyword.
// The regex can spuriously match control-flow statements such
// as "else if (...) {" (returnType="else", name="if"); skip any
// match whose captured name is a keyword.
static const std::unordered_set<std::string> glslKeywords = {
"if", "else", "for", "while", "do", "switch", "case", "return", "break", "continue", "discard", "struct"};
if (glslKeywords.count(match.str(2)))
{
searchBegin = match[0].second;
continue;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And the unordered_set needs to be included:

Suggested change
#include <regex>
#include <unordered_set>

Let me know what you think @tjjackson

@bernie-laberge bernie-laberge changed the title fix: Bugfix/aces2 support fix: SG-43607: Bugfix/aces2 support Jun 8, 2026
@bernie-laberge bernie-laberge added PR: Acknowledged New PR has been acknowledge by the TSC community Contribution from the Open RV Community PR: Planned_P1 PR will be review and set soon. Expect 1 to 4 weeks delay. and removed PR: Acknowledged New PR has been acknowledge by the TSC labels Jun 8, 2026
@bernie-laberge bernie-laberge added PR: In Progress PR is being reviewed by code reviewer. and removed PR: Planned_P1 PR will be review and set soon. Expect 1 to 4 weeks delay. labels Jul 3, 2026
@bernie-laberge

Copy link
Copy Markdown
Contributor

Note that I was able to reproduce this issue with a standard config so I created an issue with the repro steps in it for QA:
#1330

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community Contribution from the Open RV Community PR: In Progress PR is being reviewed by code reviewer.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants