TweakPlug : Possible way to expose functionality needed by PrimitiveVariableTweaks #6154
Conversation
include/Gaffer/TweakPlug.h
Outdated
| @@ -123,6 +123,15 @@ class GAFFER_API TweakPlug : public Gaffer::ValuePlug | |||
| MissingMode missingMode = MissingMode::Error | |||
| ) const; | |||
|
|
|||
| template< typename T > | |||
| static void applyNumericTweakValue( | |||
There was a problem hiding this comment.
Maybe just applyNumericTweak(), and return the result rather than pass dest by reference?
There was a problem hiding this comment.
I've done this, though I didn't like using the same name for this and the other function named applyNumericTweak. I realized the other function is private though, so I've just renamed it applyNumericDataTweak ... is that reasonable?
include/Gaffer/TweakPlug.inl
Outdated
| @@ -179,4 +181,95 @@ bool TweaksPlug::applyTweaks( | |||
| return tweakApplied; | |||
| } | |||
|
|
|||
| template<typename T> | |||
| T vectorAwareMin( const T &v1, const T &v2 ) | |||
There was a problem hiding this comment.
Seems a bit odd for this to be in a public namespace. Could either put it in a Detail sub-namespace, or make it a private static method of TweakPlug?
There was a problem hiding this comment.
Made private.
include/Gaffer/TweakPlug.inl
Outdated
| // These cases are unused - we handle them outside of numericTweak. | ||
| // But the compiler gets unhappy if we don't handle some cases. | ||
| assert( false ); | ||
| break; |
There was a problem hiding this comment.
I think this warrants an exception - now its in the public API, we can't make the same guarantee.
There was a problem hiding this comment.
Addressed.
include/Gaffer/TweakPlug.inl
Outdated
| throw IECore::Exception( | ||
| fmt::format( | ||
| "Cannot apply tweak with mode {} to \"{}\" : Data type {} not supported.", | ||
| modeToString( mode ), tweakName, "foo" //sourceData->typeName() |
There was a problem hiding this comment.
Some strangeness here we'll need to tidy up.
There was a problem hiding this comment.
I've gone with staticTypeName, with a comment about the downsides, maybe this should have been a TODO.
danieldresser-ie
force-pushed
the
tweakPreReq
branch
from
b799b57
to
ddab904
Compare
|
Addressed feedback. Added one more commit where I made modeToString public, since that's pretty useful for primvar handling where we can't use I've hit the "ready for review" button rather than leaving this as draft, since I guess it could be merged now. Though it would also be fine to leave it until I put up PrimitiveVariableTweaks for review, since nothing else is affected by this, and it's possible I'll find more things that need changes along the way. |
PrimitiveVariableTweaks will need to apply tweaks to array elements, which are not
Data. I'm exploring ways to expose this functionality in TweakPlug without duplicating code.This appears to work somewhat decently ( all the tests are passing ). My concerns are: what should this public function be called, and would it be better not to expose vectorAwareMin/vectorAwareMax ( these functions are only called in one place, so just pasting them in is totally an option ).
Any better ideas for an interface?