MeshSplitter

[danieldresser-ie]

Hopefully this API matches what you're expecting based on prior conversations.

Known outstanding issues:

• Currently a failing test because when I reimplemented MeshAlgo::segment, I did it the way that seems more natural to me, where passing a primitive variable value that doesn't exist gets you back a null pointer, rather than a mesh with 0 faces. Should I implement this the old way, to return a mesh with 0 faces instead ... and I suppose set up all the primitive variables with zero sized data as well?
• Should there be any special case for the primvar that you are splitting on? Currently, it is treated like any other primvar, though it will always end up with every value being the same ... I guess this is fine, though I almost wonder whether it should become a constant rather than a uniform?
• My big set of perf tests are messy in terms of naming and code duplication, and are two slow to run regularly. Should I clean up this large set of tests, or maybe just keep this big set saved somewhere locally for if I ever revisit the algorithm, and just commit a lighter set of perf tests more like what we would use for monitoring regressions in CI ( if the Cortex CI had a way to hook that up?

[danieldresser-ie]

Quick note in case you are examining the crease test closely: testing out this data interactively in Gaffer, and actually evaluating the creases using IE OpenSubdiv node, revealed a bug, but thankfully it was just a typo in the tests - this caused some of the creases to be wrong to start with, and get even more wrong after splitting. Will be a quick fix.

Would also be nice to fix the bogus creases that remaining when two vertices are kept, but all faces connecting them are removed, but that is a known issue with the previous DeleteFaces version, so I guess it doesn't need to be done now.

[johnhaddon]

I can't say I've got a high degree of confidence in my review, but I have tried my best to follow through all the logic, and don't see any errors. Most of my comments are about changes that I think would aid readability. There are some pretty long functions here, which aren't necessarily bad on their own, but I found myself struggling to remember the meaning of various generically named variables throughout - "index" can mean so many things.

I think though, the biggest change that would have aided my comprehension would have been to first write a functioning all-tests-passing version without all the different optimisation branches, and then introduce those in separate commits with justification in the form of stats. Maybe one for next time.

I'd like to do some interactive testing of this in Gaffer, as I think that's my best chance of finding any potential bugs, so perhaps you could get the associated Gaffer PR up before we merge this one?

[danieldresser-ie]

I think I have now addressed or replied to all comments, and added a new PR with splitBounds. There are a couple of test failures - one because I used something from itertools that I didn't realize was Python 3 ( Cortex isn't fully on 3 yet, is it? Maybe there's something in six to address it, I'll check tomorrow ). The other is a compiler warning that as far as I can tell is simply bogus ... that might require some investigation to coax the compiler to shut up about it.

[johnhaddon]

Thanks Daniel - couple of additional comments inline but I think this is close enough to squash down at the same time as addressing them.

Cortex isn't fully on 3 yet, is it?

Nope, it's stuck in 2/3 limbo. We need to bump major version to ditch 2. @ivanimanishi, would it present a problem for you if we did this in a Gaffer 1.3 timeframe?

The other is a compiler warning that as far as I can tell is simply bogus

Definitely looks bogus to me. I wonder if moving the throw into an else block would make a difference?

[johnhaddon]

Think this one escaped range-forification in your last push...

[danieldresser-ie]

Addressed the last couple of comments.

Also implemented sorting of results, removed the need for the templated constructor, rearranged the API some based on that, and renamed things based on our conversation this morning.

Outstanding issues:

There are some level details in the implementation of the freshly rearranged initializeFaceToSegments that perhaps call for consideration ( I think this code has probably gotten a bit clearer from the API reorg, but it has independently gotten more complicated to add the sorting ) : could the code for sorting be shared more between the indexed and unindexed cases? Does it make sense to now use the remapSegmentIndices vector to perform the sorting in the the unindexed case, or since the unindexed case is allocating a new faceToSegmentIndexBuffer anyway, should it bake the sort into the index buffer? I don't think any of this really matters much though, we probably could just roll it out like this.

Also noticed: I'm not checking the canceller in initializeFaceToSegments and I probably should be. But there is a tension between preferring ranged-for and our classic "check the canceller every 10000 elements you process". Should I be writing code like:

for( auto &d : data )
{
    if( ( &d - &data[0] ) % 10000 == 0 )
    {
        Canceller::check( canceller );
    }

Also, setting up a Python binding for value() required making an accessor for the primitive variable so I can check its type. Any better ideas?

[danieldresser-ie]

Does it make sense to now use the remapSegmentIndices vector to perform the sorting in the the unindexed case

Answering one of my own questions: yes, I should just always use remapSegmentIndices. It is now needed fairly consistently, and freed when the constructor finishes anyway, and making it used always instead of usually will remove some special cases from the code.

[danieldresser-ie]

Added a final cleanup PR where I try to tidy up the internal logic some, add Canceller checking, and added a manual definition of accumulate for Python 2.

That seems to have gotten the tests passing - something about how I rearranged it seems to have made MSVC happy.

I think this is pretty good to go in theory, though the history is now pretty messy - not sure whether I should be squashing it before you review.

[johnhaddon]

I haven't looked at this in as much detail as the first review, and there's a lot of churn in the new commits, but it all works nicely for me in Gaffer. Of the comments I made inline, only one has survived (the others were made irrelevant by subsequent commits) - could you address that in some way while squashing down for merging please?

[johnhaddon]

Is this really a todo? I'd just change the comment I think :

// Used by the python binding, but otherwise shouldn't be necessary

Alternatively, we could declare the function in the binding to be a friend?

[danieldresser-ie]

I've gone with the updated comment. I originally tried to make the binding function a friend, but I couldn't figure out how to declare it as a friend without giving the existence of the binding function public visibility.

[danieldresser-ie]

Updated the comment, and gave everything a big squash.

[johnhaddon]

I failed to notice that this was targeting main before I merged it, so I've also merged it to RB-10.4 manually.