Skip to content

Return the geomprop from mx_geompropvalue_filename - #3063

Open
Chang-Jin-Lee wants to merge 1 commit into
AcademySoftwareFoundation:mainfrom
Chang-Jin-Lee:fix-osl-geompropvalue-filename
Open

Return the geomprop from mx_geompropvalue_filename#3063
Chang-Jin-Lee wants to merge 1 commit into
AcademySoftwareFoundation:mainfrom
Chang-Jin-Lee:fix-osl-geompropvalue-filename

Conversation

@Chang-Jin-Lee

Copy link
Copy Markdown

Fixes #3003.

mx_geompropvalue_filename never returned the fetched geomprop. getattribute writes into its destination on success and returns 0 only when the attribute is absent, so the assignment sat in the wrong branch:

string defaultFilename = defaultVal.filename;
if (getattribute(geomprop, defaultFilename) == 0)
    out.filename = defaultFilename;

On success nothing copied defaultFilename into out. On failure the assignment was a no-op, because out = defaultVal on the line above had already done it. Either way out.filename stayed at the default.

The reporter suggested == 1, which does work, since that first line covers the fallback. I went with the shape the ten sibling implementations in libraries/stdlib/genosl/ already use — every one tests failure and installs the default there, and the composite types copy the fetched value in an else:

// mx_geompropvalue_vector2.osl
if (getattribute(geomprop, value) == 0)
    out = defaultVal;
else { out.x = value[0]; out.y = value[1]; }

The local is renamed too, since defaultFilename holding a fetched value is arguably what made this easy to miss.

Testing

$ MaterialXTest "[genosl]"
All tests passed (2498 assertions in 5 test cases)

$ MaterialXTest
All tests passed (26198 assertions in 84 test cases)

I should be straight about the limit of that: I have no OSL renderer available, so the runtime behaviour is not executed here. The case rests on getattribute's documented contract and on matching ten siblings in the same directory. If you would rather see a render-side check before merging, say so and I will find a way to run one.

Fixes AcademySoftwareFoundation#3003. The OSL implementation never returned the fetched geomprop.
getattribute writes into its destination on success and returns 0 only when
the attribute is absent, so the assignment sat in the wrong branch:

    string defaultFilename = defaultVal.filename;
    if (getattribute(geomprop, defaultFilename) == 0)
        out.filename = defaultFilename;

On success nothing copied defaultFilename into out. On failure the assignment
was a no-op, because out = defaultVal on the line above had already done it.

The ten sibling implementations in the same directory all test failure and
install the default there, with the composite types copying the fetched value
in an else branch. This follows that shape, and renames the local since it
holds a fetched value rather than a default.

Verified: full MaterialXTest suite passes, 26198 assertions across 84 test
cases, including the five [genosl] cases. I have no OSL renderer available,
so the runtime behaviour is argued from getattribute's documented contract
and the sibling implementations rather than executed.

Assisted-by: Claude (Anthropic)
@linux-foundation-easycla

Copy link
Copy Markdown

CLA Not Signed

@jstone-lucasfilm

Copy link
Copy Markdown
Member

Thanks for this proposal, @Chang-Jin-Lee, and before we review it in detail, take a look at the instructions at #3063 (comment) for resolving the CLA authorization warnings.

@Chang-Jin-Lee

Copy link
Copy Markdown
Author

Thanks — following the EasyCLA link now, so the authorization is on me rather than anything blocking on your side. I'll post here once the check flips.

Everything else on the PR is green: all 37 build jobs pass, and the change is one line in mx_geompropvalue_filename.osl plus the render test that fails without it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OSL mx_geompropvalue_filename never returns the geomprop value

2 participants