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

Failure to connect Float Out plugs to Color In plugs with Cycles shader nodes #5553

Closed
abelmilanes opened this issue Nov 18, 2023 · 5 comments

Comments

@abelmilanes
Copy link

abelmilanes commented Nov 18, 2023

Version: Gaffer 1.3.7.0-linux
Third-party tools: Cycles
Third-party modules:

Description

Failure to connect Float Out plug to Color/Vector In plug with Cycles shader node. This happens even if the connection is made to individual [r,g,b] [x,y,z] values.
Also seem to happen when using nodes, but not when defining a Cycles Shader Network as an IECoreScene.ShaderNetwork() attribute via an expression.

It is possible however to connect individual [r,g,b] [x,y,z] values from Color/Vector Out plugs to Float In plugs.

Steps to reproduce

See attached gaffer script:

connection_bug.zip

Debug log

Click to Expand

WARNING : IECoreCycles::ShaderNetworkAlgo : Couldn't find socket input "color.b" on shaderNode "shader:d2a26e30a5ace686386e0b2ec1c6f291:emission1"
WARNING : IECoreCycles::ShaderNetworkAlgo : Couldn't find socket input "color.g" on shaderNode "shader:d2a26e30a5ace686386e0b2ec1c6f291:emission1"
WARNING : IECoreCycles::ShaderNetworkAlgo : Couldn't find socket input "color.r" on shaderNode "shader:d2a26e30a5ace686386e0b2ec1c6f291:emission1"

@johnhaddon
Copy link
Member

Thanks for the clear repro steps. Looks like this just isn't handled at all in the Cycles backend - we have checks for when the connection source is an [ r, g, b] or [ x, y, z ] component, but not for when the destination is. It also looks like we're a bit wasteful and make multiple conversion nodes when a component-level source is used for multiple connections. Shouldn't be too big of a job to fix.

@johnhaddon
Copy link
Member

I think the way we'll want to fix this is to add a variant of IECoreScene::ShaderNetworkAlgo::convertOSLComponentConnections() that works for any shader type, and allows you to specify what shaders to use for the intermediate conversions. Then we can call that from the Cycles backend rather than have a bunch more custom logic.

@johnhaddon
Copy link
Member

I think the way we'll want to fix this is to add a variant of IECoreScene::ShaderNetworkAlgo::convertOSLComponentConnections() that works for any shader type, and allows you to specify what shaders to use for the intermediate conversions.

At the same time as doing this, I think we want to move away from using the MaterialX shaders as the default converters for Arnold networks, and use Arnold shaders instead.

johnhaddon added a commit to johnhaddon/gaffer that referenced this issue Oct 24, 2024
We do this by registering adaptors with ShaderNetworkAlgo and then letting `addComponentConnectionAdapters()` do all the hard work for us. This also means that we are inserting the adaptors appropriately when exporting Cycles shaders to USD via the SceneWriter.

Also replaced the old handling for `color.[rgb]->float` connections with the ShaderNetworkAlgo approach. This reduces code complexity, and also means we reuse the adaptors when one source is connected to multiple destinations.

Fixes GafferHQ#5553
johnhaddon added a commit to johnhaddon/gaffer that referenced this issue Oct 24, 2024
We do this by registering adaptors with ShaderNetworkAlgo and then letting `addComponentConnectionAdapters()` do all the hard work for us. This also means that we are inserting the adaptors appropriately when exporting Cycles shaders to USD via the SceneWriter.

Also replaced the old handling for `color.[rgb]->float` connections with the ShaderNetworkAlgo approach. This reduces code complexity, and also means we reuse the adaptors when one source is connected to multiple destinations.

Fixes GafferHQ#5553
@github-project-automation github-project-automation bot moved this from Up Next to Pending release in Work in Progress Oct 28, 2024
@abelmilanes
Copy link
Author

Thank you. Just tested 1.5.0.0 and this works as expected now.

@johnhaddon
Copy link
Member

Thanks for testing!

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

No branches or pull requests

2 participants