Support Inputs To Splines #1343
Conversation
johnhaddon
left a comment
johnhaddon
left a comment
There was a problem hiding this comment.
I think the only comment I made regarding correctness was the query about duplicating connections, but I made a bunch of other comments related to making this more readable, since I have to admit to struggling with it.
What else is missing before we can consider merging? Tests and USD support?
|
I was humming and hawing about some of this reorganization to try and better fit what you're looking for on Friday, but I realized it's probably more useful if I at least upload it so you have the option of taking a look if you want to. |
danieldresser-ie
force-pushed
the
rampInputs
branch
from
a54bd63 to
f7a8a75
Compare
|
I've squashed everything up until when you last glanced at this code, which hopefully makes it easier to review the new stuff, which does involve some rearrangement. As far as I know, the only outstanding thing is that I haven't tested Appleseed, since we don't have an Appleseed install handy here. |
|
The test failure is probably just due to the Appleseed test not being updated for the last commit where I set the unused extra values for OSL to -1, because it is testing the unused values. Should be an easy fix. Also, I forgot to rename concatStrings to concat, but maybe you can figure out what to call intFromStringView and I can rename that at the same time. |
danieldresser-ie
force-pushed
the
rampInputs
branch
2 times, most recently
from
fe97c2d to
c90ec86
Compare
|
OK, I've now updated the string helper names. I also resolved the Appleseed test, though it did reveal one difference in behaviour: previously, when the renderer backends were calling expandSplineParameters themselves, they could do that on all shaders, not just OSL shaders. But when the backend just calls convertToOSLConventions, it only processes OSL shaders. I think this is reasonable - there are no current examples of where a non-OSL shader would have a spline parameter ... but it's something to think about ( along with it being a good idea for someone with Appleseed handy to briefly test it interactively ). |
|
Note : I've changed the base of this PR to |
johnhaddon
left a comment
johnhaddon
left a comment
There was a problem hiding this comment.
Thanks Daniel. I've added a few comments inline - mostly nitpicks but I think perhaps one instance of undefined behaviour? And I'm afraid I didn't find the culprit in my review, but in my testing in Gaffer I'm also finding that Constant interpolation mode is broken now, with or without input connections. Can provide an example script if you need, but it should be pretty simple to repro...
| const IECore::CompoundDataMap &targetParameters = targetShader->parameters(); | ||
|
|
||
| int targetSplineKnotOffset = -1; | ||
| auto findTargetParameter = targetParameters.find( splineName ); |
There was a problem hiding this comment.
This naming occurs in a few places and tripped me up each time - find is a verb, so findTargetParameter sounds like a function rather than a variable. Is there anything wrong with targetParameter or targetParameterIt?
There was a problem hiding this comment.
Renamed to targetParameterIt
There was a problem hiding this comment.
There are a whole load more of these - findValues, findSplineff etc. I'm going to merge anyway, but might be worth bearing in mind...
|
|
||
| void ShaderNetworkAlgo::convertToOSLConventions( ShaderNetwork *network, int oslVersion ) | ||
| { | ||
| std::map< IECore::InternedString, std::pair< IECore::InternedString, size_t > > currentSplineArrayAdapters; |
There was a problem hiding this comment.
I wasn't really thinking about performance, but since you brought it up, I would have assumed two things : that constructing an empty map doesn't allocate, and that adding members after clear() doesn't reuse previous allocations. The former appears to be true for GCC/Clang but perhaps not MSVC, but I think the latter holds for all of them?
convertToOSLConventions
danieldresser-ie
force-pushed
the
rampInputs
branch
from
c90ec86 to
db60740
Compare
|
Addressed comments in place. Replied to all of them, except the currentSplineArrayAdapters one which github won't let me reply to, but I did just move the constructor inside the loop for simplicity. Sorry for the confusion on the issue with constant splines ... it turns out I had gotten used to Gaffer where I always do a full build while testing, and hadn't realized that the last Cortex build I did to test the final duplication commit was just a scons testScene, and it didn't actually install. So turns out I could reproduce the issue fine ... I think it technically is sort of an OSL bug, but the easy work around was just to revert the last commit, and add a comment about why we don't need to duplicate connections for Y. |
2 participants
Less fully considered than the corresponding Gaffer PR, but putting it up so you can see this half of it while looking at the Gaffer side.