Fix oslquery link conflict #1348
Conversation
In PR 1346, we overlooked the fact that OSLQuery::init() is defined *differently* in liboslquery and the separate copy that used to be in liboslexec. In this patch, we remove the oslquery-side version (and in fact coalesce it into the constructor, the only place it was used). So now instead of being replicated differently in the two libraries, it only exists in oslexec. That constructor, since it takes a ShaderGroup* as an argument, only makes sense to call if you have a full ShadingSystem nearby. Signed-off-by: Larry Gritz <[email protected]>
There was a problem hiding this comment.
Something feels a bit odd about having the methods of a class being split among different libraries. I agree that this seems like the simplest fix though.
Just to suggest an alternative - maybe ShaderGroup could contain a method that returns an OSLQuery object? Would that make things any clearer?
|
@fpsunflower Maybe it would. I did think of that, too. I don't know if that's more or less confusing. I can mock that up pretty quickly if you want to look at it and render a judgment. |
|
I can't get rid of this completely because it's an ABI change, and we may need to backport this fix to the release branch. What do you think about me merging this PR as it stands, and then I'll submit a separate one (for master only) that proposes deprecating the OSLQuery direct support for the shader group and makes a new ShadingSystem method instead? |
|
Updated with a proposed addition of ShadingSystem::oslquery(). Marked the OSLQuery methods as deprecated, but can't remove them until we're at an ABI-breaking version change. |
|
Any comments? |
|
One more update posted: avoid a debug mode assertion when the new ShadingSystem::oslquery encounters a parameter that is an array of indeterminate length. |
This eliminates the weirdness of having this constructor undefined in liboslquery, moves the functionality to the ShadingSystem. Because of ABI compatibility issues, we have to merely deprecate (but not remove) the OSLQuery functionality for now. Signed-off-by: Larry Gritz <[email protected]>
…nite length Signed-off-by: Larry Gritz <[email protected]>
Signed-off-by: Larry Gritz <[email protected]>
In PR 1346, we overlooked the fact that OSLQuery::init() is defined
differently in liboslquery and the separate copy that used to be
in liboslexec.
In this patch, we remove the oslquery-side version. So now
instead of being replicated differently in the two libraries, it only
exists in oslexec. That constructor, since it takes a ShaderGroup* as
an argument, only makes sense to call if you have a full ShadingSystem
nearby.
Signed-off-by: Larry Gritz [email protected]