mergePrimitives core function #6026

danieldresser-ie · 2024-09-04T07:09:20Z

Found a few other weird odds-n-ends while doing a last clean up pass today, so didn't end up getting to transferring the mesh boundary interpolation properties, but I wanted to get this up for review, and everything else in a pretty good state to be reviewed. I might consider dividing some things up into more functions - it is a big chunk of code ... but at this point, where to draw those boundaries would be pretty arbitrary, so I might as well see how you feel about it.

Oh - something I just realized - I'm now handling interpretations of Constant primvars correctly ... but I guess that's a behaviour change when I use this same behaviour for FreezeTransform. I guess if we're putting this in 1.5, then the compatibility break is OK? I'd also be OK with just not transforming Constant primvars, but the behaviour should match between MergeMeshes and FreezeTransform.

johnhaddon

Thanks Daniel!

I've reviewed as much as I've had time for, and now the smell of a curry is drifting me home-office to home-home :) Thought it was worth posting what I had so far since you'd gone to the trouble of getting this up for review yesterday rather than today.

The should-i-refactor-the-huge-function-and-if-so-how? question is a tricky one. Rather than try to answer it, I've just made a few comments about the experience as a new reader. The idea I'm somewhat drawn to is trying to refactor such that there is a top-level function per primitive type, each of which prepares PrimVarInfos and then calls a shared function that does the central threaded data-munging. The question there is what to do with the primitive-type-specific data-munging (creases etc). Maybe just do it separately? Or pass a functor to the threaded munger? I feel like a mjor benefit of this structure could be that you're either looking at type-specific code or generic code, rather than the constant back-and-forth in the current function. And you'd only be looking at a single type at a time, rather than all three types at once. I definitely don't feel confident enough about this that you should act today - might be a good topic for a conversation tomorrow?

In the meantime I've made a few requests for splitting up the unit tests, and I think I did spot one genuine bug.

Cheers...
John

python/GafferSceneTest/IECoreScenePreviewTest/PrimitiveAlgoTest.py

johnhaddon · 2024-09-04T15:29:54Z

src/GafferScene/FreezeTransform.cpp

-		transformOp->matrixParameter()->setValue( new M44fData( transform ) );
-		transformOp->primVarsParameter()->setTypedValue( primVarNames );
-		transformOp->operate();
+		IECoreScenePreview::PrimitiveAlgo::transformPrimitive( *outputPrimitive, transform, context->canceller() );


I'm now handling interpretations of Constant primvars correctly ... but I guess that's a behaviour change when I use this same behaviour for FreezeTransform. I guess if we're putting this in 1.5, then the compatibility break is OK?

Yeah, I think that's fine for 1.5. We'll just need to update Changes.md to mention it - I'd bill it as a fix, but we should probably mention it in Breaking Changes too, since its one of those ones that might catch someone out.

python/GafferSceneTest/IECoreScenePreviewTest/PrimitiveAlgoTest.py

src/GafferScene/IECoreScenePreview/PrimitiveAlgo.cpp

johnhaddon · 2024-09-04T17:21:29Z

src/GafferScene/IECoreScenePreview/PrimitiveAlgo.cpp

+				switch( var.interpolation )
+				{
+					case PrimitiveVariable::Vertex:
+					case PrimitiveVariable::Varying:
+						varInfo.needsVerts = true;
+						break;
+					case PrimitiveVariable::Uniform:
+						varInfo.needsFaces = true;
+						break;
+					case PrimitiveVariable::FaceVarying:
+						varInfo.needsVerts = true;
+						varInfo.needsFaces = true;
+						break;
+					default:
+						break;
+				}


Am I too optimistic in thinking that all this needs stuff boils down to varInfo.interpolation = max( [ var.interpolation for var in primitives ] )? Ah, maybe that falls apart for curves because Varying > Vertex?

Just trying to think of ways of limiting the type-specific stuff, or at least not having it permeate this entire function.

It's definitely not quite that simple - between Vertex > Varying, and the combination of Varying and Uniform on a mesh requiring FaceVarying. But it is possible to implement this as a mergeInterpolations function that just takes two interpolations and the prim type - the implementation of that function is fairly messy at the moment, but it does remove a bunch of messiness from elsewhere, so it seems worth it.

Addressed in ae07b51

I then tried to make the code look a bit more like a max() in this commit: 0ccada4

The first commit seems like a big win ( a bit complicated looking, but moves all the complexity to one place ). The second commit might actually make things worse though - it's more concise, but I think a bit harder to understand, since it relies on completely understanding the ordering of the enum.

Reading your comment, I was expecting to agree that the second commit was worse, but I think it actually read better to me.

src/GafferScene/IECoreScenePreview/PrimitiveAlgo.cpp

danieldresser-ie · 2024-09-05T04:52:25Z

The idea I'm somewhat drawn to is trying to refactor such that there is a top-level function per primitive type, each of which prepares PrimVarInfos and then calls a shared function that does the central threaded data-munging. The question there is what to do with the primitive-type-specific data-munging (creases etc).

I was expecting to argue against introducing specialization via functors or templating to something that doesn't actually need to be generic, but having thought about what it would look like, maybe it would be fine. Though I think I would focus more on handling the assembly of the result. The preparation of PrimVarInfo doesn't actually need to depend much on the prim type ( especially if do the mergeInterpolations function ), I'm thinking more of something like templating on a MeshResults struct that could hold the topology/creases, and have functions that add the specific results ( and possibly static functions if we need to customize other things per type ). Haven't worked out the details, but seems like it might be fine.

Sounds like you want to expose 3 different functions in the API? I don't care much whether it's one wrapper that calls the correctly specialized inner functions, or 3 different wrappers. If it is 3 different wrappers, are you still OK with it being PrimitiveAlgo, or do I need to make it a private header and then call it from MeshAlgo/CurvesAlgo/PointsAlgo?

danieldresser-ie · 2024-09-06T05:11:41Z

The main refactor did end up being pretty substantial, but I think it moves things a lot more in the direction you were wanting:
243bc77

As far as I know, other than a squash, the only things that are missing are:

optimization of transformPrimitiveVariables - very simple, was hoping to get to it today, maybe I can do that Monday along with squashing
more in depth threading of mergePrimitives - this is more complicated, plan is to leave it for later.

danieldresser-ie · 2024-09-10T05:37:41Z

OK, all comments now replied to, and I've threaded transformPrimitive. I took a quick look at more granular threading of mergePrimitives, but concluded there's not really any point until we come up with some way to skip the pointless zero-initialize of *VectorData ( which we can't really multithread ).

johnhaddon

Thanks Daniel! I've not gone through the refactoring in complete detail, but I like the gist of it and we have good test coverage so I think that's good enough. I did make a few comments about how we might tweak it slightly to make it even clearer, including one suggestion that might render my earlier comments about mergeInterpolations() redundant. I think we're in a good place to get this squashed down and read for merging, so could you rebase and retarget to main when doing that please. Also please update Changes.md to mention the breaking change in FreezeTransform (and note that I've suggested mpving the threading for FreezeTransform to a separate RP, where we can more thoroughly consider the cache policy changes needed).
Cheers....
John

python/GafferSceneTest/IECoreScenePreviewTest/PrimitiveAlgoTest.py

johnhaddon · 2024-09-10T15:22:20Z

src/GafferScene/IECoreScenePreview/PrimitiveAlgo.cpp

-			}
+			std::vector< Imath::V3f >& writable = vecVar->writable();
+
+			tbb::parallel_for(


Now we're using TBB here, we'll need a TaskCollaboration task policy in the FreezeTransform node. That's not super straightforward though, since we don't want that policy to apply to the simple pass-through computes when the filter doesn't match. We'd need to offload the threaded compute onto an internal plug. I wonder if we can derive from ObjectProcessor for that?

Since this isn't the main focus of this PR, I'd like to suggest we save this for a follow-up PR (meaning we drop the TBB here for now). I'm also not sure if we considered cache policies in the MergeObjects base class - can you remind me?

By the way, what kind of speedups did you get from the parallel_for in transformPrimitive()?

parallel_for gave me about an 8X speedup in transformPrimitive - it's extremely parallelizable. I wonder if it would make sense to keep the fast implementation of transformPrimitive and put anything necessary to disable threading in FreezeTransform ... actually, wait, do we need to do anything special in FreezeTransform. If it's not using TaskCollab, it won't benefit from threading, but will it cause any problems?

It would benefit from threading even without the right cache policy, but I don't have the headspace to reason about the deadlock-inducing possibilities of it not being protected by task isolation and all the other TaskCollaboration shenanigans. This PR is about mergePrimitives(), so please let's keep it about that. I don't expect the FreezeTransform changes to be huge, but I would benefit from considering them separately (and it keeps you moving on the main goal - the MeshMerge node).

johnhaddon · 2024-09-10T15:34:19Z

src/GafferScene/IECoreScenePreview/PrimitiveAlgo.cpp

+			// promoted to FaceVarying.
+			assert( destInterp == PrimitiveVariable::FaceVarying );
+
+			const MeshPrimitive *sourceMesh = IECore::runTimeCast< const MeshPrimitive >( sourcePrim );


I don't think I really buy that argument - having primTypeId != sourcePrim->typeId() would be a coding error, and narrowing the interface to make that impossible seems worthwhile to me. But it's all internal, so I'll leave it up to you...

johnhaddon · 2024-09-10T15:49:52Z

src/GafferScene/IECoreScenePreview/PrimitiveAlgo.cpp

+		// If they're the same, then we don't need to promote.
+		result = a;
+	}
+	else if( a == PrimitiveVariable::Constant || ( primType != IECoreScene::MeshPrimitiveTypeId && a == PrimitiveVariable::Uniform ) )


Uniform interpolation on a CurvesPrimitive isn't effectively constant - it's one per curve. I feel like this should cause a genuine problem when a is uniform and b is constant - won't we return constant in that case?

Maybe it is actually wrong, and the next commit sweeps it under the rug? Or maybe I just missed something...

Oh, this is fine for correctness, but confusing: we later promote constant to uniform on Curves, since Constant wouldn't allow for different values from different source prims. So treating Constant as the same as Uniform is fine here. But sounds like you prefer the second version anyway.

johnhaddon · 2024-09-10T15:59:03Z

src/GafferScene/IECoreScenePreview/PrimitiveAlgo.cpp

+				switch( var.interpolation )
+				{
+					case PrimitiveVariable::Vertex:
+					case PrimitiveVariable::Varying:
+						varInfo.needsVerts = true;
+						break;
+					case PrimitiveVariable::Uniform:
+						varInfo.needsFaces = true;
+						break;
+					case PrimitiveVariable::FaceVarying:
+						varInfo.needsVerts = true;
+						varInfo.needsFaces = true;
+						break;
+					default:
+						break;
+				}


Reading your comment, I was expecting to agree that the second commit was worse, but I think it actually read better to me.

src/GafferScene/IECoreScenePreview/PrimitiveAlgo.cpp

johnhaddon · 2024-09-10T16:14:54Z

src/GafferScene/IECoreScenePreview/PrimitiveAlgo.cpp

-	else if( resultCurves )
+}
+
+IECoreScene::PrimitivePtr PrimitiveAlgo::mergePrimitives(


It's interesting that the implementation now basically switches on type at the very top, meaning that my preferred type-safe mergeMeshes()/mergeCurves()/mergePoints() API would no longer be an artificial construct on top of the main implementation. Simpler to leave things as they are for now though, rather than faff around separating things into different headers including a private one...

src/GafferScene/IECoreScenePreview/PrimitiveAlgo.cpp

…primvars

danieldresser-ie · 2024-09-10T23:45:00Z

Addressed the last round of comments, squashed, added changelog entries, retargeted to main.

danieldresser-ie requested a review from johnhaddon September 4, 2024 07:09

johnhaddon reviewed Sep 4, 2024

View reviewed changes

danieldresser-ie mentioned this pull request Sep 6, 2024

MergeObjects #5984

Merged

johnhaddon reviewed Sep 10, 2024

View reviewed changes

danieldresser-ie force-pushed the mergePrimOnly branch from 99271ae to 12518e5 Compare September 10, 2024 23:35

danieldresser-ie changed the base branch from 1.4_maintenance to main September 10, 2024 23:35

danieldresser-ie added 2 commits September 10, 2024 16:36

MeshAlgoTessellateTest : Crease support for assertMeshesPraticallyEqual

d12da2b

MeshAlgoTessellateTest : betterAssertAlmostEqual : Support lists and …

bb01d9a

…primvars

danieldresser-ie force-pushed the mergePrimOnly branch from 12518e5 to 381550a Compare September 10, 2024 23:37

danieldresser-ie added 2 commits September 10, 2024 16:43

PrimitiveAlgo : Added transformPrimitive and mergePrimitives

306970f

FreezeTransform : Use new PrimitiveAlgo

5d8789b

danieldresser-ie force-pushed the mergePrimOnly branch from 381550a to 5d8789b Compare September 10, 2024 23:43

danieldresser-ie marked this pull request as ready for review September 10, 2024 23:44

danieldresser-ie mentioned this pull request Sep 10, 2024

PrimitiveAlgo : Optimize transformPrimitive #6037

Merged

johnhaddon merged commit 66215e1 into GafferHQ:main Sep 11, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mergePrimitives core function #6026

mergePrimitives core function #6026

danieldresser-ie commented Sep 4, 2024

johnhaddon left a comment

johnhaddon Sep 4, 2024

johnhaddon Sep 4, 2024

danieldresser-ie Sep 5, 2024

danieldresser-ie Sep 6, 2024

johnhaddon Sep 10, 2024

danieldresser-ie commented Sep 5, 2024

danieldresser-ie commented Sep 6, 2024

danieldresser-ie commented Sep 10, 2024

johnhaddon left a comment

johnhaddon Sep 10, 2024

danieldresser-ie Sep 10, 2024

johnhaddon Sep 10, 2024

johnhaddon Sep 10, 2024

johnhaddon Sep 10, 2024

johnhaddon Sep 10, 2024

danieldresser-ie Sep 10, 2024

johnhaddon Sep 10, 2024

johnhaddon Sep 10, 2024

danieldresser-ie commented Sep 10, 2024

mergePrimitives core function #6026

mergePrimitives core function #6026

Conversation

danieldresser-ie commented Sep 4, 2024

johnhaddon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldresser-ie commented Sep 5, 2024

danieldresser-ie commented Sep 6, 2024

danieldresser-ie commented Sep 10, 2024

johnhaddon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldresser-ie commented Sep 10, 2024