SceneAlgo::hierarchyHash

[danieldresser-ie]

I'm feeling fairly good about this overall, and how it fits in with what we're planning for Instancer.

I don't really like the current structure of this code though - my initial implementation was pretty clean, but it incorrectly included hashes of full path, not relative paths ( which would cause problems when we want to use this for Instancers with relative prototypes ). To fix that, I really want to do a parallel reduce, similar to what you needed in USDLayerWriter. I realized I can actually do a reduce using the way that functors are allocated by parallelProcessLocations, which is sort of neat, but this workaround is not good for clarity. If we're not currently adding a parallelGatherLocations, I should probably just implement my own walk function, same as is currently done in USDLayerWriter.

The constraint that we want identical subhierarchies to hash the same is an interesting one - we require different valeus to have different hashes, but we don't generally require the same value to have the same hash. I suspect there are a lot of ways of constructing identical hierarchies where their hashes would end up being different, but this code does now seem to be working when the hashes of each element are consistent. ( When I first tested it, I was testing on the root location, and then tried to get the hash of a variety of stuff at the root to match the same stuff under a group ... but I wasn't able to get it to match. I suspect this is because evaluating the objectPlug at the root is a special case, and its hash will never match another location - even if that location doesn't have any object either. Anyway, for now I've just changed the test to compare two groups, one of which is nested an extra layer deep, and those do have matching hashes. )

[johnhaddon]

Thanks Daniel! The SceneAlgo stuff is feeling pretty good to me - a few comments inline, but not much. Your comments indicate that the performance test isn't any good - would be good to sort that out. USDLayerWriter stuff also seems like it needs a bit of tidying up, and benchmarking against the original - could perhaps do that in a separate PR?

[danieldresser-ie]

OK, I think I've now addressed all feedback, and added Changelog entries.

I've also added a comment to the test, and I'll clarify a bit more here what I meant by the perf test being "not very good":
It's just very dominated by the time to actually hash the plugs, so none of the rest of what it's doing is very noticeable - when I was looking at the old approach, and comparing an atomic to thread local storage, neither made any perceptible difference. In particular, the sort of thing I would like this test to show is that it's the right decision to use parallel_reduce instead of parallel_deterministic_reduce ... it completely fails to show that, there is absolutely no difference. It still feels like a reasonable decision to use parallel_reduce ( using simple adds to combine hashes in an order-independent way is an approach we're relying on elsewhere, so it should be fine here, and USDLayerWriter doesn't rely on order ) ... but I haven't really been able to prove any concrete benefit.

[danieldresser-ie]

I should flag one last change: I've also added an extra test for hierarchyHash that tries to ensure the internal parallelReduceLocations mechanism is functioning properly.

[johnhaddon]

Thanks Daniel!