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

Delight Renderer : Register cloud rendering variant #5833

Merged

Conversation

johnhaddon
Copy link
Member

image

@gkocov, I can't add you as a reviewer without you being a member of the GafferHQ team on GitHub, but I'd welcome your input anyway. I've also sent you an invite to be a "collaborator" on GitHub which I think would add you to the list of potential reviewers in future - feel free to accept or decline as you see fit.

@johnhaddon johnhaddon self-assigned this Apr 30, 2024
@gkocov
Copy link
Collaborator

gkocov commented Apr 30, 2024

Thanks @johnhaddon! Accepted the invite and will look into the PR and do some tests with the CI builds later today.

@johnhaddon johnhaddon requested a review from gkocov April 30, 2024 16:01
@johnhaddon
Copy link
Member Author

Accepted the invite and will look into the PR and do some tests with the CI builds later today.

Great, thanks! I've added you as a reviewer now too...

@gkocov
Copy link
Collaborator

gkocov commented May 1, 2024

Tested both under Linux and Windows and everything LGTM.

One thing to note is that currently 3Delight Cloud on Windows has a critical bug - when 3Delight accesses certain files for upload to the cloud it runs into the following error which prevents the normal functioning of the cloud render:

3Delight Cloud: unsuccessful upload, status is 4, message is 'Unable to parse ExceptionName: InvalidChunkSizeError Message: Only the last chunk is allowed to have a size less than 8192 bytes'

The error doesn't happen under Linux and is not related to anything that Gaffer is doing. Houdini on Windows runs into the same issue as well. Reported the error to Aghiles and the 3Delight team are looking into it.

The addresses we pass into the parameter list need to be valid until the `NSIBegin()` call returns. I think it's time we started to switch over to the NSI C++ API - the C API is too error-prone.
In an ideal world perhaps, we'd have implemented this as an option specified by DelightOptions, but that would put us in the chicken-and-egg situation where we can't make an NSI context without the cloud option, and we can't handle the other options until we have an NSI context. Although there _are_ strategies for dealing with that (see CyclesRenderer) they're complex and it would be preferable to avoid them. So instead this is implemented by registering a second "3Delight Cloud" renderer type, with the justification being that cloud rendering is a different enough operation that it's reasonable to present it to a user this way. In practicals terms there is no difference between this and a potential option, because the renderer can be specified using the standard `render:renderer` option.
@johnhaddon
Copy link
Member Author

Tested both under Linux and Windows and everything LGTM.

Thanks very much for testing! Merging now...

The error doesn't happen under Linux and is not related to anything that Gaffer is doing.

Ah, good to know that it's a general problem - fingers crossed for a quick fix. Thanks for investigating...

@johnhaddon johnhaddon merged commit 78f85bf into GafferHQ:1.4_maintenance May 1, 2024
5 checks passed
@johnhaddon johnhaddon deleted the delightCloudRenderer branch May 1, 2024 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants