OVDB-18: Remove deprecated use of AttributeArray::set() in points/PointMove.h #319
Conversation
Signed-off-by: Dan Bailey <danbailey@ilm.com>
openvdb/points/PointMove.h
Outdated
| SingleCopyIterator singleCopyIterator; | ||
| singleCopyIterator.setIndices(sourceOffset, targetOffset); | ||
| targetArray.copyValues(sourceArray, singleCopyIterator); | ||
| #else |
There was a problem hiding this comment.
There's a lot of unnecessary virtual methods being called here. copyValues calls loadData on both source and targte, and expand and compact on the target. You can alleviate the call to compact with the last argument but that still leaves the others. It seems that what you really want is to loop over all source arrays and load/expand the data once, then call copyValuesUnsafe() with range checking as true. I think I mentioned this in the original PR for this implementation.
There was a problem hiding this comment.
I'll look at doing what you're suggesting here. Bear in mind, the main purpose of this PR is to eliminate the use of a deprecated function in this (very rare) code branch for ABI=6+. The current ABI=5 implementation is a lot of code but essentially provides similar performance to the newer method.
Ultimately, what I'd really like to do is wait until ABI=5 is mostly no longer being used then do a big refactor of this whole logic to simplify the code dramatically and then leave ABI=5 using the slow virtual function copying method for backwards-compatibility only.
There was a problem hiding this comment.
Sure, though the removal of sourceArray.loadData() and passing false to copyValues for compact should definitely be done imo. Why is this code branch a very rare case? I just happened to choose this change set to comment on, but it applies to the other one in this PR too. I can see a case of wanting to copy arbitrary (potentially single) indices from multiple source arrays to a single target array being common (e.g. point merge by groups) and think it's good that this is highlighted now as it does seem a fairly obvious limitation.
edit: a follow up, do we even need range checking here? If not then for this particular case we can just call copyValuesUnsafe(), though I wasn't sure.
There was a problem hiding this comment.
It's a rare case, because for all registered types (which includes all types available in Houdini), this logic doesn't get invoked at all. It purely exists for the cases where you're registering and using an attribute type which isn't registered by default.
I'm going to do the aforementioned refactor now to produce a more tailored ABI=6 version. It's a bit of code duplication, but the ABI=6 one can be much simpler as a result of the introduction of AttributeArray::copyValues().
There was a problem hiding this comment.
Thanks for clarifying, that sounds good to me. Though is there any reason why rangeChecking couldn't be exposed as an argument on copyValuesUnsafe? Seems that that would resolve this too
openvdb/points/PointMove.h
Outdated
|
|
||
| #if OPENVDB_ABI_VERSION_NUMBER >= 6 | ||
| sourceArray.loadData(); | ||
| SingleCopyIterator singleCopyIterator; |
There was a problem hiding this comment.
could move the construction of SingleCopyIterator out of this loop and in all other usages in this file
There was a problem hiding this comment.
I'd be very surprised if the compiler doesn't do this optimisation. Reason for putting it in the loop is to reduce the number of #if statements littering the code without incurring unused variable warnings.
There was a problem hiding this comment.
Sure, this is minor, I think it's clearer regardless of what we assume the compilers doing (at the cost of more #if statements) but I'll leave it up to you.
Signed-off-by: Dan Bailey <danbailey@ilm.com>
|
@Idclip - updated this PR with two major changes:
My intention is to leave this code as-is until ABI=6+ usage is significantly more common than ABI=5, then I'll drastically simplify the ABI=5 logic to remove the code that attempts to work-around the performance issues related to the use of the virtual functions (until we can finally deprecate ABI=5 in years to come) |
|
Sounds great @danrbailey thanks for doing this now. I'll take a look early next week |
|
|
||
| using IndexTriple = std::tuple<LeafIndex, Index, Index>; | ||
| using IndexTripleArray = tbb::concurrent_vector<IndexTriple>; | ||
| using GlobalPointIndexMap = std::vector<IndexTripleArray>; | ||
| using GlobalPointIndexIndices = std::vector<IndexArray>; |
There was a problem hiding this comment.
All these typedefs are becoming confusing, we should really implement a class which holds these values with a sensible API to help people follow this code (specifically for GlobalPointIndexMap, GlobalPointIndexIndices and LocalPointIndexMap). Perhaps not required for this PR
There was a problem hiding this comment.
I agree, I do think it's a bit confusing.
I tried a couple of alternatives, but couldn't come up with anything that I felt was much clearer. Ultimately, it's all internal logic and not exposed via the public API, so I didn't spend too much time on it, but certainly open to suggestions for making this better. I'll merge this PR for now though, thanks for reviewing.
There was a problem hiding this comment.
Ultimately, it's all internal logic and not exposed via the public API, so I didn't spend too much time on it
No worries, but will help with future reviews too :)
No description provided.