Skip to content

Conversation

@jstone-lucasfilm
Copy link
Member

@jstone-lucasfilm jstone-lucasfilm commented Jan 8, 2025

Description

This changelist contains two fixes for OSL builds on the Windows platform, both of which were authored by Kai Rohmer at NVIDIA.

Tests

With these fixes in place, OSL builds on Windows now include testshade and testrender, where these two applications would previously fail to build.

For visual validation, I'm attaching the latest MaterialX render comparisons between GLSL and OSL, where the OSL renders are using the codebase in OSL main plus these two fixes.

MaterialXRenderTests_01_07_2025_GitHub.pdf

Checklist:

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
  • My code follows the prevailing code style of this project. If I haven't
    already run clang-format v17 before submitting, I definitely will look at
    the CI test that runs clang-format and fix anything that it highlights as
    being nonconforming.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 8, 2025

CLA Not Signed

This changelist contains two fixes for OSL builds on the Windows platform, both of which were authored by Kai Rohmer at NVIDIA.

Signed-off-by: Jonathan Stone <[email protected]>
@jstone-lucasfilm
Copy link
Member Author

As an update on the Contributor License Agreement, it sounds like Lucasfilm may not have contributed to OSL before, so it's going through the approval process now!

Comment on lines +5 to +6
set (LLVM_COMPILE_FLAGS ${LLVM_COMPILE_FLAGS} -std=c++${CMAKE_CXX_STANDARD})

Copy link
Collaborator

Choose a reason for hiding this comment

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

I ran into this as well, but there is actually a better solution!

@lgritz
Copy link
Collaborator

lgritz commented Jan 12, 2025

I think my Windows CI PR is going to subsume this. I still think you should push the CLA through the bureaucracy so you've got the ducks in a row for the next time somebody from ILM needs to make an OSL patch. But I suspect this PR is ultimately not going to be merged.

@jstone-lucasfilm
Copy link
Member Author

No problem, Larry, and a set of Windows fixes directly from the OSL team is even better from my perspective. I'll close out this PR, but will continue with the Lucasfilm approval process so that we can contribute in the future.

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.

2 participants