PrimitiveVariableTweaks

[danieldresser-ie]

Feels like I've been working on this for too long without eyes on it, so I wanted to get up something for you to look at.

As far as I know, this is pretty well good to go, though I do have a few ideas on more useful corner case behaviour I will discuss tomorrow ( and the handling of indexed primvars when an idList is specified was a very last minute addition ... I only realized that was probably the right way to handle it while finishing up the test cases, but it does seem correct ).

I think you should now be able to look at the main things that might require iteration - the refactor of TweakPlug, and the naming of things in the UI. I'm not a fan of the name "AllElements", but I couldn't come up with something better.

[danieldresser-ie]

I didn't quite get to adding new tests for the new selectionMode functionality, but in case you get a chance to review this today, I have updated the implementation and UI with the new selectionMode features discussed this morning.

[danieldresser-ie]

OK, tests have been updated.

In theory this would be all ready to go, except for one last corner case I found at the last minute while testing, noted here:

# TODO - this is some pretty nasty behaviour that probably needs fixing. If an id occurs twice in
# the list, it gets tweaked twice. This is probably unexpected ( and it's definitely unexpected that
# it gets tweaked twice if there is no id primVar, but only once if there is an id primvar ). So we
# probably need to do something about this, though it's pretty unfortunate that this will mean
# unnecessarily sticking all the ids in an unordered_set in the common case where the id list doesn't
# contain duplicates.

A shame from a performance point of view, but I should probably fix that.

[johnhaddon]

Thanks Daniel! This is gonna be handy, and I think the interpolation/selectionMode logic has turned out pretty nicely.

Other comments inline as usual. I haven't reviewed "Expose applyValueTweak" particularly thoroughly because I want to consider another possible way of factoring it first - see my comments on TweakPlug.h.

Cheers...
John

[johnhaddon]

Are we losing GeometricData::interpretation() here, and if so, does it matter? Should we have been taking it from currentValue rather than newData all along?

[johnhaddon]

PrimiitiveVariableTweaks -> PrimitiveVariableTweaks

[johnhaddon]

This is talking about option when it should be talking about primitive variables.

[johnhaddon]

Choose -> Chooses. We need a description of each mode here too.

I see that selection mode is available when the interpolation is Constant, but I haven't been able to make a sparse edit to a constant array - I get errors like the following :

`Cannot apply tweak to "test" : Variable data of type "IntVectorData" does not match parameter of type "IntData".`

I can't think of a reason why this shouldn't be possible, but if it isn't then we should reflect that in the UI.

[danieldresser-ie]

Oh, I was initially confused by this ... looks like I omitted my final round of changes from this PR. Fortunately, the only things missing were some minor formatting fixes, and a UI fixup where I did update the UI to reflect the current behaviour.

I'm open to discussing changing this behaviour, but it would make everything a bit more complicated. Currently the rule is: "When tweaking a Constant primvar, use a tweak that matches the type of the primvar. When tweaking any other interpolation, use a tweak that match the type of one element of the primvar." If we want to support per-element tweaks of Constant vector primvars ( I assume this would include both using an IdList to set certain elements, or something like using a Color3f plug to add a value to every element of a Color3fVector constant ), that would be a new code path. Currently, you can only apply ListAppend, ListPrepend, ListRemove value tweaks to vector Constants. Do you have a use case in mind to warrant the extra complexity?

[johnhaddon]

This might need tweaking, depending on the outcome of my comment above about the selection mode and Constant interpolation.

[johnhaddon]

The NodeEditor is quite busy with all the preceding options. I wonder if we'd benefit from a divider before the tweaks, or from putting the tweaks in a section (open by default)?

[johnhaddon]

Since we can tweak "P", we should probably derive from ObjectProcessor so we can get the bounds right. CopyPrimitiveVariables gives an example of doing this without invoking the cost of bounds propagation unless "P" is actually written to. Alternatively, we could refuse to write to "P" for now.

[johnhaddon]

I think this can be a one-liner?

idList = new Int64VectorData( vector<int64_t>( intData->readable().begin(), intData->readable().end() ) );

[johnhaddon]

Question : would it make sense for the mask to be the canonical representation, and to convert the id list into that form? With the code as it is, perhaps that'll be less efficient, but we still have to deal with not tweaking the same ID twice - feels like converting to the mask does that for us reasonably efficiently?

[johnhaddon]

I'm not super comfortable with this way of factoring things, for a few reasons :

• It leaves us duplicating a lot of the TweakPlug logic in a separate component, and that logic is not the most straightforward.
• I think other TweakPlug clients could plausibly benefit from array operations (masked or unmasked).
• We've managed to serve the needs of lots of different clients with a single API so far, and that API has successfully encapsulated all of the complexity (although I admit ShaderTweaks isn't as straightforward).

So, questions :

• Could the existing applyTweak() function be modified to do non-masked operations on arrays, benefiting all existing clients?
• Could an additional masked operation abstract away all the stuff to do with primitive variables? Maybe something like the below?

/// As above, but applying the tweak to individual elements of VectorTypedData,
/// as specified by `mask`.
template<class GetDataFunctor, class SetDataFunctor>
bool applyMaskedTweak(
	GetDataFunctor &&getDataFunctor,
	SetDataFunctor &&setDataFunctor,
        const vector<bool> &mask,
        // Size of array to make for `Create` mode.
        size_t createSize,
	MissingMode missingMode = MissingMode::Error
) const;