-
Notifications
You must be signed in to change notification settings - Fork 218
Remove Old Compatibility For Scripts Saved With Gaffer 0.40 and earlier #6735
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
base: main
Are you sure you want to change the base?
Conversation
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.
I'd be quite happy to leave it at 0.40 for now. I'm sure a case can be made to go further, but I'd rather not have to think too much about possible unexpected downsides right now. It feels nice to be making a bit of progress, but without pushing further than required by the spline work for now... |
|
Oh, and LGTM. |
|
I've now added 3 commits modifying tests to get them passing - this does emphasize that while any scripts saved after 2017 shouldn't have these problems, it is possible for any code to have them, even code written recently ( for example, someone setting a Color4f plug with a Color3f value is something someone writing Python pipeline code could have done recently ). The fixes are straightforward, so this is probably fine, but it will be something to be aware of when it comes time to roll this out, and we probably should be quite explicit in the changelog. I'm thinking something like: Breaking
|
Working on top of #6717, remove any other remaining compatibility configs targeting files from Gaffer 0.40 and earlier.
Just putting up a first pass on this for discussion.
My process here was just to run
gitk startup/*/*Compatibility.pyand scroll back to the beginning of the log to find compatibility configs created before 2018, and then quickly audit them to make sure their purpose hadn't changed since.The choice of 0.40 as a cutoff is pretty arbitrary - I guess we'd discussed it just because that was the vintage of the spline compatibility shim I wanted to remove to reduce confusion with the new shim. If we wanted to extend this a bit farther into 2018, the next major ones on the chopping block would be supporting CompoundPlug, and support PathMatcherData from before it moved to Cortex ... any thoughts on where a logical demarcation would be?