Replace SplineDefinition with IECore::Ramp, and Store RampData in Shaders #6717

danieldresser-ie · 2025-11-27T00:13:03Z

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.

johnhaddon

Thanks Daniel! There's a colossal amount of change here (it pretty much brought my web browser to its knees), but I think it all makes sense. Let's chat about what we want to do with ColorSpline.osl/ColorRamp.osl compatibility in our meeting later today...

johnhaddon · 2025-11-27T15:04:03Z

startup/Gaffer/splinePlugCompatibility.py

-Gaffer.SplineffPlug.__getitem__ = __getitemWrapper( Gaffer.SplineffPlug.__getitem__ )
-Gaffer.SplinefColor3fPlug.__getitem__ = __getitemWrapper( Gaffer.SplinefColor3fPlug.__getitem__ )


I think removing this is OK - out intention has always been to remove compatibility configs after a grace period has elapsed. Looks like its version 0.40.0.0 and earlier that we're breaking from, which seems reasonably far in the past. We'll need to note this in Changes.md.

Once this is merged we should probably also have a quick look through the other compatibility files, and removing anything supporting a version earlier than 0.40.

Put up a first pass PR addressing other early compatibility files: #6735

src/IECoreDelight/ShaderNetworkAlgo.cpp

johnhaddon · 2025-11-27T15:09:26Z

src/IECoreArnold/Renderer.cpp

-					writeAccessor->second = new ArnoldShader( shader, m_nodeDeleter, m_universe, nameFormat, m_parentNode );
+					// Make sure we don't leave empty entries in the cache
+					m_cache.erase( writeAccessor );
+					throw;


What is it that is getting thrown, and where is it ending up? Generally speaking we don't throw from the Renderer API.

I've now fixed the specific issue that was triggering this in Cortex. Does this mean I should discard this commit, since in theory nothing should be throwing exceptions here? ( As discussed, there are in fact still a bunch of ways that expandRamps can throw exceptions, but I guess we're planning on fixing those )

I think I'd be inclined to remove it, but I could probably be persuaded either way.

Now removed.

include/Gaffer/RampPlug.h

python/GafferUI/RampPlugValueWidget.py

shaders/Pattern/ColorRamp.osl

johnhaddon · 2025-11-27T16:04:18Z

startup/GafferOSL/shaderNameCompatibility.py

+	"Pattern/FloatSpline" : "Pattern/FloatRamp",
+	"Pattern/ColorSpline" : "Pattern/ColorRamp",


I wonder if it's worth not having this, since we're keeping the old shaders around. It would let us make what would otherwise be breaking changes in the new shaders (like the parameter name change proposed above). I wonder if there are any other changes you'd make if you were rewriting ColorRamp from scratch?

As per our conversation offline, I've currently gone in the other direction - adding an additional compatibility shim for the parameter name change proposed, and adding a test: 7f6b70f

Everything seems to be working fine.

startup/GafferOSLUI/splineUIMetadata.py

include/Gaffer/RampPlug.h

danieldresser-ie · 2025-12-04T02:57:18Z

Currently working through the details of these comments, mostly pretty straightforward.

While looking at your comments about renaming "spline" -> "ramp" in variable names, I did some grepping for "spline" ... there are actually a bunch more places where a rename makes sense ... not going to help if your browser was struggling with the size of this PR before, but it does make sense to address them at the same time.

The only things that came up potentially needing discussion:

The rename of ColorSpline/FloatSpline to ColorRamp/FloatRamp: if I recall correctly, the proposal last we talked about this is that we'd go all in on compatibility shims - I'll write some tests for backwards compatibility, and as long as those work, we'll go ahead and do the rename, and change the value of the default for direction, and I guess in response to your comment, do a childAlias to rename the parameter spline to ramp as well. I suppose there is a question about how much we're actually gaining, versus just leaving the ColorSpline in place in old scenes, but if we ever want to get rid of ColorSpline, it's probably pretty worthwhile to actually do the compatibility conversions.
The exception currently being thrown while rendering. I guess this should be a msg instead? The reason I made it an exception is that I was trying to match the old behaviour, and previously this was an exception thrown when evaluating the ShaderAssignment. Now that the representation used in the ShaderNetwork has no problems representing connections to MonotoneCubic ramps, there's no real reason to throw while evaluating the ShaderAssignment, so the problem is caught later. Oh ... I just realized that this might require a change in the Cortex code that I was hoping would get merged today. Should IECoreScene::ShaderNetworkAlgo::convertToOSLConventions never throw? Or should we be catching whenever we call it in Gaffer? Is the expectation that connections to MonotoneCubic ramps should be ignored, and emit a warning? Before, they would halt the render, but that was because evaluating the scene would fail when it got to the ShaderAssignment.

danieldresser-ie · 2025-12-06T02:57:28Z

I think I've now addressed all comments ... hopefully it doesn't bog your browser down too much, since you were saying this PR was already giving you issues for some reason. If it would help, I can try to pull anything that isn't a simple rename out of the big rename commit - though I'd probably do that after squashing 59f2572 into the big rename.

danieldresser-ie · 2025-12-09T02:26:42Z

Squashed last round of fixes, added changelog entries, switched dependencies to Cortex-10.7.0.0a1.

I should mention one fix that I've squashed that wasn't in the last round of fixes to review: I must not have tested the most recent version of IECoreArnoldTest, because the last round of tests of testOSLShaders were trying to compare the addresses of shaders connected to the duplicated endpoints, and that was failing - I've switched to checking that the properties of the connected shaders are correct, and that seems to be working fine now.

This should be good for interactive testing, and as far as I know, basically ready to merge.

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.

danieldresser-ie · 2025-12-16T17:16:15Z

I've removed the platform23 test. Unfortunately still got two test failures, but they seem unrelated.

danieldresser-ie requested a review from johnhaddon November 27, 2025 00:13

danieldresser-ie mentioned this pull request Nov 27, 2025

Replace SplineDefinition with IECore::Ramp, and Store RampData in Shaders #6716

Closed

danieldresser-ie force-pushed the splinesRound2 branch from 0c8defb to 2939020 Compare November 27, 2025 00:16

murraystevenson assigned danieldresser-ie Nov 27, 2025

github-project-automation bot moved this to Pending Review in Work in Progress Nov 27, 2025

murraystevenson added this to Work in Progress Nov 27, 2025

johnhaddon reviewed Nov 27, 2025

View reviewed changes

danieldresser-ie force-pushed the splinesRound2 branch 2 times, most recently from b67c436 to 4cbd7e5 Compare December 8, 2025 23:23

danieldresser-ie mentioned this pull request Dec 9, 2025

Forward compatibility for Gaffer 1.7 RampPlugs / shaders #6734

Open

danieldresser-ie force-pushed the splinesRound2 branch from 4cbd7e5 to 7b76754 Compare December 9, 2025 02:20

danieldresser-ie marked this pull request as ready for review December 9, 2025 02:26

danieldresser-ie mentioned this pull request Dec 9, 2025

Remove Old Compatibility For Scripts Saved With Gaffer 0.40 and earlier #6735

Draft

danieldresser-ie added 11 commits December 15, 2025 18:27

Update Cortex dependency version to 10.7.0.0a1

f59ed1b

IECoreDelight::ShaderNetworkAlgo : Use convertToOSLConventions

89a6c7b

Previously, connections to color components in OSL shaders would fail in 3Delight

Update type blacklists to support building against Cortex-10.7

948e506

GafferImage::Ramp : Drop "using namespace IECore" to prevent conflict

c0dfc26

IECoreRenderMan : IECoreScene::ShaderNetworkAlgo::expandRamps renamed

1cefa29

SplinePlug : Remove old compatibility config

41b8aa9

Rename ColorRamp and FloatRamp osl shaders

d385ed1

Use IECore::Ramp, and rename all "splines" that are now "ramps"

6cc90d3

Remove linux-gcc11-platform23 CI target

dfe1d95

danieldresser-ie force-pushed the splinesRound2 branch from baddb88 to dfe1d95 Compare December 16, 2025 02:27

		Gaffer.SplineffPlug.__getitem__ = __getitemWrapper( Gaffer.SplineffPlug.__getitem__ )
		Gaffer.SplinefColor3fPlug.__getitem__ = __getitemWrapper( Gaffer.SplinefColor3fPlug.__getitem__ )

		"Pattern/FloatSpline" : "Pattern/FloatRamp",
		"Pattern/ColorSpline" : "Pattern/ColorRamp",

Replace SplineDefinition with IECore::Ramp, and Store RampData in Shaders #6717

Are you sure you want to change the base?

Replace SplineDefinition with IECore::Ramp, and Store RampData in Shaders #6717

Conversation

danieldresser-ie commented Nov 27, 2025

Uh oh!

johnhaddon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danieldresser-ie commented Dec 4, 2025

Uh oh!

danieldresser-ie commented Dec 6, 2025

Uh oh!

danieldresser-ie commented Dec 9, 2025

Uh oh!

danieldresser-ie commented Dec 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants