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

IECoreRenderMan Windows #6285

Merged
merged 3 commits into from
Feb 25, 2025
Merged

Conversation

ericmehl
Copy link
Collaborator

This adds the basics to get IECoreRenderMan running and passing tests on Windows. As with #6271, it doesn't run tests on CI yet. The use of ctypes in 8c1dff8 should be removable when we export at least one symbol from _IECoreRenderMan.pyd Python module, so that Windows will actually import the symbols from the IECoreRenderMan.dll file that registers the renderer.

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Gaffer project's prevailing coding style and conventions.

Copy link
Member

@johnhaddon johnhaddon 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 taking the initiative on this one Eric! It's very nice to be able to stay in my Linux bubble as much as possible :)

Couple of questions inline, and one thing I think we need to fix regarding linux compatibility...

Cheers...
John

bin/gaffer.cmd Outdated
) else (
set RMANTREE_STRIPPED=%RMANTREE%
)
call :appendToPath "!RMANTREE_STRIPPED!" GAFFER_EXTENSION_PATHS
Copy link
Member

Choose a reason for hiding this comment

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

Is the usage of GAFFER_EXTENSION_PATHS what prompted your comment about spamming the environment a bit? Otherwise, the variables are the same as on Linux, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it was the usage of GAFFER_EXTENSION_PATHS. It sets up a few paths like the glsl path that the Linux wrapper isn't setting, and I wasn't sure if that was done to keep those out of the environment variables when we know there won't be shaders and such there. Should be harmless enough?

But yes, otherwise my intention is to keep them the same on each platform.

Copy link
Member

Choose a reason for hiding this comment

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

otherwise my intention is to keep them the same on each platform.

As an aside, I've been wondering how much of the separate Linux/Windows wrappers we could consolidate into the shared __gaffer.py. It seems like potentially quite a lot? Would be interested in any insights you might have there...

This spams the environment variables a little bit, but I think this is
better than needing `add_dll_directory()` in
`IECoreRenderMan.__init__()` and it's the same as we're doing for
Arnold.
We can't hold onto the `lightAttributes` longer than the renderer or it
will attempt to delete a shader from a null Riley pointer when cleaning
up the `lightAttributes`.
On Windows, we need to close the image before attempting to delete the
temporary files in `tearDown()` so the OS will let them be deleted.
@ericmehl
Copy link
Collaborator Author

Comments are addressed and I went ahead and squashed down. Ready for a new look!

@johnhaddon johnhaddon merged commit 6d20bc9 into GafferHQ:1.5_maintenance Feb 25, 2025
5 checks passed
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