Cycles : Fixed potential crashes when using render: attributes on objects due to passing invalid data.

[boberfly]

Generally describe what this PR will do, and why it is needed

• Object-level attributes must always be padded to TypeFloat4 currently, and OIIO made ParamValue not always be a nullptr for data that can fit in a pointer footprint in its implementation. These I think only worked and got triggered in OSL and not SVM where they aren't supported.

• Fixed potential crashes when using render: attributes on objects due to passing invalid data, checking ParamValue via .data() for nullptr would fail in cases where the struct stores data in its footprint instead of a pointer.

• Prior to this a lot of "mysterious" crashing or glitched shaders would appear, this addresses many of them like Cycles UDIM texture issues in OSL mode (UVs?) #6315.

• Just to note my unit-test was made in a hurry and might not trigger the crash, however it probably will render correctly when it didn't before.

• Added float/color3f/color4f vector of size 1 support to be a constant value - many USD/Alembic caches exported out of other DCCs tend to do this instead of not making it a vector unfortunately.

Related issues

• Cycles UDIM texture issues in OSL mode (UVs?) #6315

Dependencies

• N/A

Breaking changes

• N/A, removed matrix types which wouldn't have worked anyways.

Checklist

• I have read the contribution guidelines.
• I have updated the documentation, if applicable.
• I have tested my change(s) in the test suite, and added new test cases where necessary.
• My code follows the Gaffer project's prevailing coding style and conventions.

[boberfly]

Any tips on making the warning as expected? Tried a bunch of things to no avail... AssertionError: Unexpected message : WARNING : IECoreCycles::Renderer : Custom attribute "unsupportedMatrix" has unsupported type "M44fData".

[murraystevenson]

Any tips on making the warning as expected? Tried a bunch of things to no avail... AssertionError: Unexpected message : WARNING : IECoreCycles::Renderer : Custom attribute "unsupportedMatrix" has unsupported type "M44fData".

I'd think this would do it: self.ignoreMessage( IECore.Msg.Level.Warning, "IECoreCycles::Renderer", "Custom attribute \"unsupportedMatrix\" has unsupported type \"M44fData\"." )

Is this TypeFloat4 padding necessary with Cycles 5.1? At this point I think we're better off focusing any Cycles efforts on main. From a cursory inspection it looks like TypeFloat and TypeFloat2 are handled - or am I looking in the wrong place? Do we also get anything out of this switch from TypeRGBA to TypeFloat4?

[boberfly]

Hey @murraystevenson in my testings I've been loading many caches with loads of attributes and found cycles to be incredibly unstable in OSL-mode prior to this change and it goes back to that issue I flagged awhile ago when I didn't understand why at the time, as well as getting visually incorrect shaders. #6315

For reference I am basing it off discussions I've had with @brechtvl to advise doing it this way, and checking how Blender does things: https://projects.blender.org/blender/blender/src/commit/4db51e9d1e1e2e7f9e96e9305607824c39f7b554/intern/cycles/blender/object.cpp#L423

I think this brings value to remove one less mystery bug in GafferCycles 1.6/Cycles 4.5 so I pointed it at 1.6_maintenance. I'm also looking to add some fixes on Cycles itself to protect against bad param values (or preferably just add support for more including arrays and matrices) but not for 4.5, looking into 5.2+ for that.

Any tips on making the warning as expected? Tried a bunch of things to no avail... AssertionError: Unexpected message : WARNING : IECoreCycles::Renderer : Custom attribute "unsupportedMatrix" has unsupported type "M44fData".

I'd think this would do it: self.ignoreMessage( IECore.Msg.Level.Warning, "IECoreCycles::Renderer", "Custom attribute \"unsupportedMatrix\" has unsupported type \"M44fData\"." )

I'll try that fix you suggested, thanks!

[johnhaddon]

Just to note my unit-test was made in a hurry and might not trigger the crash, however it probably will render correctly when it didn't before.

I ran the test 100 times without the fix in place, and it passed 100 times. It would give us a lot more confidence that this change is correct if the test showed the original problem.

it goes back to that issue I flagged awhile ago when I didn't understand why at the time, as well as getting visually incorrect shaders. #6315

On that issue you say "Alright I think I've got to the bottom of this - render:displayColor for this asset is a single value in a Color3fVectorData which causes OSL to have issues but not SVM oddly.". That makes it sound like the issue was fixed by the treat-an-array-of-1-as-a-single-value change, is that right? So the everything-as-float4 change is needed for something else? Having these two in the same commit isn't ideal.

Object-level attributes must always be padded to TypeFloat4 currently

Padded, or converted? It looks like ParamValue's internal storage is already big enough to house a float4, so I'd expect padding to be taken care of already. And this PR isn't padding, it's converting everything to a Float4. A bit more clarity would be useful here too.

[boberfly]

Just to note my unit-test was made in a hurry and might not trigger the crash, however it probably will render correctly when it didn't before.

I ran the test 100 times without the fix in place, and it passed 100 times. It would give us a lot more confidence that this change is correct if the test showed the original problem.

I wasn't too sure how to reenact this as it manifests when using cycles in an interactive session eg. tumbling around, changing frames, etc. it was fairly crash-prone in this state.

it goes back to that issue I flagged awhile ago when I didn't understand why at the time, as well as getting visually incorrect shaders. #6315

On that issue you say "Alright I think I've got to the bottom of this - render:displayColor for this asset is a single value in a Color3fVectorData which causes OSL to have issues but not SVM oddly.". That makes it sound like the issue was fixed by the treat-an-array-of-1-as-a-single-value change, is that right? So the everything-as-float4 change is needed for something else? Having these two in the same commit isn't ideal.

Object-level attributes must always be padded to TypeFloat4 currently

Padded, or converted? It looks like ParamValue's internal storage is already big enough to house a float4, so I'd expect padding to be taken care of already. And this PR isn't padding, it's converting everything to a Float4. A bit more clarity would be useful here too.

I think the most important change is testing for nvalues() == 1 on ParamType instead of .data() != nullptr and not passing bad data to cycles as it doesn't check for these properly and filling up the device kernel with garbled data. I'm cool for replacing this PR with that change only (and removing the matrix ones), although I don't know how to trigger the issue in a unit test like I was seeing interactively, should I just contain this fix and strip the rest?