Replace SplineDefinition with IECore::Ramp, and Store RampData in Shaders #6716
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Linear splines in OSL only need 1 extra endpoint. This test had linear splines with 2 extra endpoints, because it started from a Cortex representation that already had an extra endpoint - that's conceptually wrong, IECore::Spline only uses duplicate end points for curve types where it is required for evaluation, not for linear.
Previously, connections to color components in OSL shaders would fail in 3Delight
IECoreScene::ShaderNetworkAlgo::expandSplineParameters was the deprecated way to handle spline parameters. IECoreArnold::ShaderNetworkAlgo is now doing the up to date approach - there is a call to IECoreScene::ShaderNetworkAlgo::convertToOSLConventions in preprocessedNetwork, which will have already converted splines. As far as I can tell, this means that expandSplineParameters was currently not doing anything, and the fact it was still being called was simply an oversight.
Contributor
Author
|
Closing in favour of #6717, because I remembered that this sort of PR should be made from the central repo so that we have the option to run renderer tests ... even though that's totally hypothetical in this case until we get an updated Cortex in the dependencies. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In combination with the recent Cortex PR: ImageEngine/cortex#1499,
this implements the new way of working with shader ramp parameters that we've been discussing.
There are a lot of changes here, but most are quite trivial - it all follows pretty directly from the Cortex changes. A lot of just renaming things, though there are a few places where I was actually able to cut down on code duplication.
The commit sequence is structured so that after the first 5 commits, this will build against the new Cortex, while having made as few changes as possible ( this was handy when checking behaviour changes ).
The piece I'm most skeptical of is "FIX : ColorRamp and FloatRamp : Fix actual default value", which is why I made it a separate commit ... maybe that's getting too tricksy with the compatibility config? The advantage of this approach is that once all production files have been produced with new Gaffer, we could get rid of the compatibility config, and things would be clean, rather than needing to keep around splineUIMetadata.py for eternity.