- Notifications
You must be signed in to change notification settings - Fork718
Half Grid Support PR#2101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Half Grid Support PR#2101
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Migrate over core changes from PR 1730Signed-off-by: ghurstunither <62885595+ghurstunither@users.noreply.github.com>
prototyped half support
ValueType -> ComputeType computations in various tools
Adds math/HalfDecl.h to OPENVDB_LIBRARY_MATH_INCLUDE_FILES
* Removes ComputeType from tree & grid.* The same method still work with half grids. * Only now they infer the ComputeType through ComputeTypeFor<ValueType>::type.* A lot of methods now let the user choose the compute type through an optional template argument. * Defaulting to ComputeTypeFor<ValueType>::type when user omits the parameter.Note: The grid sampler's method sample (BoxSampler, etc) can now take an optional compute type template argument. I think this breaks backward compatibility on users with custom samplers because the GridSampler class will pass in the compute type to SamplerT::sample() now.Signed-off-by: ghurstunither <62885595+ghurstunither@users.noreply.github.com>
…lied.Restrict ComputeType to be computed, rather than optionally user supplied.Signed-off-by: ghurstunither <62885595+ghurstunither@users.noreply.github.com>
WhitespaceSigned-off-by: ghurstunither <62885595+ghurstunither@users.noreply.github.com>
Uses ComputeType during Renormalization to prevent catastrophic error with small voxel size.Signed-off-by: ghurstunither <62885595+ghurstunither@users.noreply.github.com>
ComputeType computations for finite differences.Signed-off-by: ghurstunither <62885595+ghurstunither@users.noreply.github.com>
Compute and return finite differences and other operators in terms of ComputeType.Signed-off-by: ghurstunither <62885595+ghurstunither@users.noreply.github.com>
Removes incorrect template arguments.Signed-off-by: ghurstunither <62885595+ghurstunither@users.noreply.github.com>
Explicitly return halfs.Signed-off-by: ghurstunither <62885595+ghurstunither@users.noreply.github.com>
double to float.Signed-off-by: ghurstunither <62885595+ghurstunither@users.noreply.github.com>
Fixes unused-local-typedef errors.Signed-off-by: ghurstunither <62885595+ghurstunither@users.noreply.github.com>
Correct variables.Signed-off-by: ghurstunither <62885595+ghurstunither@users.noreply.github.com>
Fixes more unused-local-typedef errors.Signed-off-by: ghurstunither <62885595+ghurstunither@users.noreply.github.com>
Correct return types.Signed-off-by: ghurstunither <62885595+ghurstunither@users.noreply.github.com>
Signed-off-by: apradhana <andre.pradhana@gmail.com>
Signed-off-by: apradhana <andre.pradhana@gmail.com>
Vec3.Signed-off-by: apradhana <andre.pradhana@gmail.com>
Signed-off-by: apradhana <andre.pradhana@gmail.com>
Signed-off-by: apradhana <andre.pradhana@gmail.com>
Signed-off-by: apradhana <andre.pradhana@gmail.com>
Signed-off-by: apradhana <andre.pradhana@gmail.com>
…hology, Grid, LevelSetUtil, RayIntersector, etc. Based on commit6add394.Signed-off-by: apradhana <andre.pradhana@gmail.com>
…y adding openvdb::common_type specialization for math::half promotion.Signed-off-by: apradhana <andre.pradhana@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My main concern with theComputeType are the rules to which we apply it. At the moment these seem a bit ambiguous, or at least I don't see a clear pattern. The documentation on the compute type doesn't relate to specific algorithms or classes, it's pretty broad. This has resulted in some cases where theComputeType is propagated between functions and returned in public API, in others it is returned as the ValueType (before being passed to anotherComputeType method), and even inComputeType templated methods it doesn't always apply to all arithmetic (cast back and forth between ValueType). You could also make the argument that, with the current definition, it should apply to the internals of all container methods (vector dot/cross/mmult/etc) but this currently isn't the case. I think if we nail down the rules as to how this is applied then we can easily review and identify issues - as it stands it's difficult to know what to suggest throughout all the arithmetic changes.
I also note that there are no unit test changes, so I assume none of the new implicitly supported serialization code or compatible tools are being actively tested with half grid types?
I am leaning towards Dan's suggestion of splitting out theComputeType implementation from the registered types/serialization, but keen to hear other thoughts.
| using BoolMetadata = TypedMetadata<bool>; | ||
| using DoubleMetadata = TypedMetadata<double>; | ||
| using FloatMetadata = TypedMetadata<float>; | ||
| using HalfMetadata = TypedMetadata<math::half>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Use your Half alias in Types?
| using half = internal::half; | ||
| }}} | ||
| #endif | ||
| #include<openvdb/math/HalfDecl.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Was there any reason for this change?
| using Vec4I = math::Vec4<Index32>; | ||
| using Vec4f = math::Vec4<float>; | ||
| using Vec4H = math::Vec4<math::half>; | ||
| using Vec4H = math::Vec4<Half>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Did you consider adding alias for half matrices? Or rather, was there a specific reason we only add aliases for vec2/3/4s?
| returnstatic_cast<ComputeType>(static_cast<ComputeType>( | ||
| A1*(2.0*f1 -7.0*f2 +11.0*f3) + | ||
| A2*(5.0*f3 -f2 +2.0*f4) + | ||
| A3*(2.0*f3 +5.0*f4 -f5))/(6.0*(A1+A2+A3))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Instead of doing this, why not just recurse?
template<typename ValueType>inlinetypename ComputeTypeFor<ValueType>::typeWENO5(const ValueType& v1,const ValueType& v2,const ValueType& v3,const ValueType& v4,const ValueType& v5,float scale2 =0.01f){using CT =typename ComputeTypeFor<ValueType>::type;ifconstexpr (!std::is_same_v<CT, ValueType>) {return WENO5<CT>(CT(v1),CT(v2),CT(v3),CT(v4),CT(v5), scale);}...// actual impl}
This would make the diff much smaller (potentially reducing errors), constrains the conversion logic to the invocation of the function and avoid redundant copies when the types are the same. You can apply this pattern to other places too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is a great idea. I'll look into doing this.
| /// where the error is fifth-order in smooth regions: O(dx) <= error <=O(dx^5) | ||
| template<typename ValueType> | ||
| inline ValueType | ||
| inlinetypename ComputeTypeFor<ValueType>::type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why return the ComputeType? When/how do we draw the line between the CT being an internal arithmetic type vs propagating it between function which may be involved in many fused arithmetic ops?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, this is a good question. For utilities like finite differences, stencil, and operators, when they returnedValueType, I noticed the tools that called them always immediately casted them toComputeType for further computations.
The problem then was the finite difference was calculated withComputeType, casted toValueType upon return, only to immediately be recast back toComputeType. This of course introduces rounding errors forValueType == half.
Perhaps I was a bit too presumptuous, but then I couldn't think of a reason these should ever return half precision, especially since well behaved functions can easily have numerically unstable derivatives.
My justification then was that for all other typesComputeType isValueType, and so no backward compatibility is broken.
[I realize this still doesn't answer your question of where the line in the sand is...]
| template<classTreeT> | ||
| inlinetypename TreeT::ValueType | ||
| BoxSampler::sample(const TreeT& inTree,const Vec3R& inCoord) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Here we continue to return ValueType, not ComputeT?
| using Vec3ui = Vec3<uint32_t>; | ||
| using Vec3s = Vec3<float>; | ||
| using Vec3d = Vec3<double>; | ||
| using Vec3h = Vec3<math::half>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So none of Vec3 methods (length/cross/etc) will use ComputeType? I've run into some code such as:
ComputeType(Vec3H().length());Where the intended use should probably be:
ComputeType(Vec3H()).length();This ties in to the somewhat arbitrary line that has been drawn with where the CT stops being propagated. It should probably apply to the underlying math of the types when running on grid operations, but this is not an easy distinction to make - without it it is easy to break.
I'd like others to comment if they have thoughts.
| VectorType* v =mVelocity; | ||
| for (size_t i =0; i < voxelCount; ++i, ++v) { | ||
| maxAbsV =math::Max(maxAbsV,ValueType(v->lengthSqr())); | ||
| maxAbsV =math::Max(maxAbsV,ComputeType(v->lengthSqr())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I believe you're computing the lengthsqr on the half vec here?
| stencil.getValue() - dt * vel->dot(GradT::result(map, stencil, *vel)); | ||
| result[i] = Nominator ? Alpha * phi[i] + Beta * a :a; | ||
| constComputeType a = | ||
| stencil.getValue() - dt *ComputeType(vel->dot(GradT::result(map, stencil, *vel))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
vel->dot can run at half precision?
| ValueType& s = speed[voxelIter.pos()]; | ||
| s -= target.wsSample(xyz); | ||
| s *= invMask ?1 - a :a; | ||
| s *= invMask ?ValueType(1 - a) :ValueType(a); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Arithmetic at ValueType precision
Signed-off-by: apradhana <andre.pradhana@gmail.com>
Thanks for the comments,@Idclip . I took these comments to heart and try to address them. I discussed these points in a meeting with@ghurstunither on Friday. We decided to do two things:
Hopefully between these two PRs options, we can get a version of HalfGrid in for OpenVDB 13. |
@ghurstunither : As promised, I updated this PR with all the unit-tests that I added. There are 4 unit-tests that are not passing. They are:
I'm passing this PR over to you. |
Thank you@apradhana /@ghurstunither - I would also try to get others to weigh in so it's not just my thoughts.@danrbailey@kmuseth@jmlait, any thoughts on the above? |
I agree with what you said, it is somewhat arbitrary at present. On the one hand, it's probably only half that will have mixed value/compute types for the foreseeable future, so I don't want to unnecessarily hold back this feature getting released for what is limited fallout right now. That being said, there are two aspects that concern me - (1) any attempts to refactor or change the implementation in the future may subtly adjust behavior for the half type (ie round up/down in a slightly different way) which could make it harder for people to rely on using this type in practice, (2) this could also now create a demand for adding compute type logic for any new algorithm we introduce which would slow down progress substantially. On balance, if we can come up with a slightly more consistent set of rules as to how to apply compute type, I would be happy to see this go out. |
What are the most important things that I should take-away from this PR at the code-level?
We want to make progress on making
HalfGridnative inOpenVDB. This means:HalfDecl.hthat brings ineither anOpenEXR halftype or theinternal halftype that we have inopenvdb. It's good to note at this point that this type is included inmath.handVec3.h.ExtendedRealGridTypesinTypes.h. This is used to buildNumericGridTypes, which is used to buildGridTypes. The functionopenvdb::initializemakes sure thatHalfGridis registered (by callingGridTypes::foreach<RegisterGrid>()).HalfGrid:What
type_traitsfunctions/idioms do you add?openvdb::is_floating_pointwhich inherits fromstd::is_floating_point. Itd allows specialization forHalftype.openvdb::is_signedwhich inherits fromstd::is_signed. It allows specialization forHalftype.ComputeTypeForto convertHalf -> float,Vec2H -> Vec2s,Vec3H -> Vec3s, Vec4H -> Vec4s`.What medium tests did you do?
- authoring
sphere level set- authoring
platonic solid level set- change level set background
- level set measure
- mesh to volume
- volume to mesh
- rendering half grid using
vdb_render(to test interpolation)