OSL ShadingEngine : add *_index* as user data #2067
Conversation
src/GafferOSL/ShadingEngine.cpp
Outdated
| } | ||
| else | ||
| { | ||
| return ShadingSystem::convert_value( value, type, &m_pointIndex, OIIO::TypeDesc( OIIO::TypeDesc::UINT64 ) ); |
There was a problem hiding this comment.
Is there an OSL query where the UINT64 code path works? If this path is basically non-useful, I'm wondering if it might be better to always use the Int32 bit path, but issue a warning on overflow, rather than have a silent failure.
There was a problem hiding this comment.
I just assumed I'd be able to call the u64 version but it does look non-useful - didn't actually read the OSL language docs. Isn't the return code sufficient to indicate there is a problem, if not is there existing warn N times functionality I can use?
There was a problem hiding this comment.
OSL docs seem to hedge and say that ints are "at least 32 bit", but in the implementation it looks like they are 32 bit, and looking at the source for convert_value, it doesn't appear to support anything else. The getattribute() return value could be used to detect failure from a shader, but I doubt anyone will check it for something they think of as built-in. I don't think we have an N-times functionality in the IECore::MessageHandler, but if we did, it wouldn't affect how you'd output the message - it'd be filtered after the fact.
I'm nitpicking really - just didn't like to have a code path that looks functional but isn't. Maybe we just add a comment explaining the situation and merge?
There was a problem hiding this comment.
I committed a version which emits a warning but it looks out of place as we don't do that in the other cases where we return false from RenderState::userData. I don't have strong opinions either way on this.
|
Thanks Don! |
@andrewkaufman mentioned it would be helpful if we had access the index of the vertex being shaded in OSLObject.
Called the user data _index to reduce the chance of collisions with primvars called index. Considered only adding the variable if a prim var doesn't exist but thought this was more confusing. Not very happy with this underscore convention but couldn't come up with anything neater.
InFloat can read this new _index primvar but not if the source data is defined as u64 so I attempt to use s32 before falling back to u64