[USD] Adding position interpolation to points. #214

sirpalee · 2017-05-24T01:31:31Z

Description of Change(s)

The PR adds a new function, ComputePointsAtTimes to UsdGeomPoints, including tests and python bindings.

The goal of the change is to have an easy to use interface to calculate velocity based interpolation for points, and in the case of no velocity fall back to the built-in interpolation mode. The change also updates the katana point-reader, so interpolated positions will are generated at shutter open and close. For example, with this change, Arnold in Katana interpolates particles nicely.

spiffmon

This is great, Pal! I'll follow up with you again about the last comment once we get our story straight internally.

spiffmon · 2017-05-25T21:17:54Z

pxr/usd/lib/usdGeom/points.cpp

@@ -238,4 +238,82 @@ TF_REGISTRY_FUNCTION(UsdGeomBoundable)
        _ComputeExtentForPoints);
 }

+size_t
+UsdGeomPoints::ComputePositionsAtTimes(
+    std::vector<VtVec3fArray>& positions,


Apologies for not documenting them yet, but our coding conventions call for "out" parameters to be passed by pointer, so that it is obvious reading call sites which are which.

Thanks for the heads up. I'll keep this in mind for the next time.

spiffmon · 2017-05-25T21:25:28Z

pxr/usd/lib/usdGeom/points.cpp

+    VtVec3fArray velocities;
+
+    // We need to check if there is a queriable velocity at the given point,
+    // if it has almost the same lower time sample and the array lengths are equal.


Why is it important that velocity be sampled at the same stride as points? I get that in most FX-exported data it will be, but it seems legit to establish a constant velocity field, or one that varies more or less slowly than the points? Velocity varying faster than the points is problematic, I get, since we'd need to establish some integration rules.

If we do want to start out trying to limit algorithmic complexity (which I'll buy!) then I don't think it's sufficient to require the lower-bound sample ordinates to match: you must also require the upper-bounds to match, also, lest your requested sampleTimes cross over a new sample of velocities.

Right, I forgot about the uses of a constant velocity field, or varying frequency. Updated the code to require both times to match.

spiffmon · 2017-05-25T21:39:09Z

pxr/usd/lib/usdGeom/points.cpp

+
+        const auto idsAttr = GetIdsAttr();
+        VtArray<long> ids;
+        if (!idsAttr.Get(&ids, sampleTimes[0])) { return 1; }


I don't think that unauthored ids should result in failing to try to interpolate. Fixed topology for any PointBased should be legit and blurrable?

More broadly, these ID checks is the only thing preventing this lovely computation from being promotable to PointBased. We definitely sometimes have varying topology meshes with authored velocities in our pipeline, and I'd love for Katana (and Hydra) to just be able to call this computation for all PointBased. With the unfortunate lame-o disclaimer that we still haven't gotten to adding infrastructure for it (though momentum is building for its need), these ID checks seem better suited to validation, which would be something you could initiate from usdview or a script.

Sure. I'll remove the ID checks for now, and just use the built-in interpolation for points. I'll check for point lengths though. Also, this means I can move the function back to pointBased. (my original implementation was there, but I promoted it to points because of the extra id checks)

spiffmon · 2017-05-25T21:50:19Z

pxr/usd/lib/usdGeom/points.h

+    /// In the case of fallback to traditional interpolation, the function will try to
+    /// confirm that the Ids are consistent across multiple position samples. In case
+    /// of missing Ids, only the first sample will be filled out. Otherwise, the function
+    /// will go as long as it can find a consistent "topology" for the points.


Just a note to update docs if we change behaviors

spiffmon · 2017-05-25T21:51:13Z

pxr/usd/lib/usdGeom/testenv/testUsdGeomPoints.cpp

+            !GfIsClose(e1[1], e2[1], epsilon) ||
+            !GfIsClose(e1[2], e2[2], epsilon)) {
+            return false;
+        }


We do specialize GfIsClose for GfVec3f, so you could simplify this function.

Thanks! I missed that. Rewrote the code to use the specialization.

spiffmon · 2017-05-25T23:39:05Z

third_party/katana/lib/usdKatana/readPoints.cpp

+    // we can use the size of this vector as a simple test for whether motion
+    // blur is enabled, without needing access to the cook interface.
+    const std::vector<double>& motionSampleTimes =
+        data.GetUsdInArgs()->GetMotionSampleTimes();


There is actually important logic in PxrUsdKatanaUsdInPrivateData::GetMotionSampleTimes() beyond what the UsdInArgs version does. I think you should call it here on the "points" attr. That said, there is some craziness regarding IsMotionBackwards() that we are trying to work through right now, so that we can get the PointInstancer and your new code doing the right thing in both forwards and backwards motion blur (we do backwards here, but want to make sure not to foist that decision on everyone). I'll contact you again when we get it sorted, which should hopefully be soon.

Thanks. Let me know once it's done, and I'll update the change asap.

OK, I think it's actually pretty simple, given how you've structured the computation so nicely to take 'baseTime' as a separate input to the sample times.

const std::vector<double> motionSampleTimes = data.GetMotionSampleTimes(points.GetPointsAttr()); // Instead of consulting shutterOpen/shutterClose to build up sampleTimes, iterate through // motionSampleTImes, adding currentTime to each element, since the fn returns frame-relative // times

I plan to lobby to get this moved up to applying to all PointBased prims in Katana, but probably OK to leave it just on Points to start.

@spiffmon Done! Both using the points attr in the query and all the motion samples, not just the shutter open and close.

jtran56 · 2017-06-02T01:16:14Z

Filed as internal issue #147139.

spiffmon · 2017-06-05T22:57:58Z

Looks great, Pal - will get this on the queue for merging!

…

On 06/05/2017 03:54 PM, Pal Mezei wrote: ***@***.**** commented on this pull request. ----------------------------------------------------------------------------- In third_party/katana/lib/usdKatana/readPoints.cpp <#214 (comment)>: > @@ -34,6 +34,49 @@ PXR_NAMESPACE_OPEN_SCOPE static FnKat::Attribute +_GetPositionAttr( + const UsdGeomPoints& points, + const PxrUsdKatanaUsdInPrivateData& data) +{ + // Because of the logic used to gather these (in PxrUsdInOp::InitUsdInArgs), + // we can use the size of this vector as a simple test for whether motion + // blur is enabled, without needing access to the cook interface. + const std::vector<double>& motionSampleTimes = + data.GetUsdInArgs()->GetMotionSampleTimes(); @spiffmon <https://github.com/spiffmon> Done! Both using the points attr in the query and all the motion samples, not just the shutter open and close. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#214 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF7qaCj759b7_ZwrJUUj39EMYE6XiyB4ks5sBIcegaJpZM4NkfW3>.

spiffmon · 2017-06-18T17:45:48Z

Hi Pal,
As one of our developers was looking to your pull request as inspiration for how to improve the readPointInstancer.cpp logic, we noticed there's an important case that's not being handled properly with respect to authored samples. This probably happens more often for PointInstancers and with Points, but the case is that data for all attributes is only authored at UsdTimeCode::Default(), and we shouldn't penalize such assets from being able to use ComputePositionsAtTimes().

Concretely (and this is why I didn't catch it at first), it means changing how you use the results of GetBracketingTimeSamples() - the last out parameter is not telling you there's no value, only that there are no timeSamples. What we're proposing is that in the case where points are authored but have no timeSamples, is to treat it exactly as if there were no velocity, as your "else" clause for "if (velocityExists)" already does exactly what we'd expect for this state of authored data (i.e. it will give you back N identical samples).

Does that make sense?

Thanks!
--spiff

sirpalee · 2017-06-19T01:02:48Z

Yes, absolutely! I'll get the PR updated soon, and keep this in mind for future PRs.

spiffmon · 2017-06-19T17:29:52Z

OK, I promise, this is the last thing! Can you add an optional, out parameter for returning the velocity vector (as VtArray) that you used, if any? And then make readPoints use that value to populate the Points location's point.v attr?

Here's why:
Our lighters often want to tweak the scale of the velocity of points, pointInstancers, etc, which is why I originally asked to add the velocityScale param to your computation. Our fx guys have written their own velocityApply op for the lighters to use, which consumes the first position sample, and velocity, and replaces the rest of the P samples with ones they compute with user-specified velocityScale applied. We would have loved to have this built into pxrUsdIn, but there's no good way to feed a user-specified velocityScale in. But at least we can ensure that our "override" velocityAply op will be using the same, correctly sampled velocity vector that your code is computing.

Thanks for bearing with me, Pal!

sirpalee · 2017-06-22T04:12:21Z

@spiffmon Updated the PR with both of the changes requested!

Let me know if you need anything else, eager to update the PR to make sure everybody is happy. :)

spiffmon · 2017-06-25T17:51:53Z

Thanks, Pal! I think the rest is on us: we are weighing two possibilities for how to continue to support authored velocityScale in Katana that is accessible to pxrUsdIn; once we have one we'll be able to pull in your readPoints.cpp code.

…

--spiff

On Jun 21, 2017, at 9:12 PM, Pal Mezei ***@***.***> wrote: @spiffmon Updated the PR with both of the changes requested! Let me know if you need anything else, eager to update the PR to make sure everybody is happy. :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

sirpalee · 2017-07-25T19:10:42Z

Sorry, pressed the wrong button.

…ationStudios#214)

spiffmon reviewed May 25, 2017

View reviewed changes

sirpalee force-pushed the pr/point_based_interpolation branch from 1b89fab to 76911ed Compare May 26, 2017 04:45

sirpalee force-pushed the pr/point_based_interpolation branch 2 times, most recently from fe177ba to 725f2d5 Compare June 5, 2017 22:52

sirpalee force-pushed the pr/point_based_interpolation branch 2 times, most recently from a8d3355 to 47606a7 Compare June 22, 2017 04:10

sirpalee closed this Jul 25, 2017

sirpalee reopened this Jul 25, 2017

sirpalee closed this Aug 14, 2017

sirpalee force-pushed the pr/point_based_interpolation branch from 47606a7 to 4ae8a56 Compare August 14, 2017 22:33

sirpalee deleted the pr/point_based_interpolation branch August 14, 2017 22:34

Felix-Kit mentioned this pull request Feb 9, 2022

Stack-overflow bug found in sdfdump #1763

Closed

AdamFelt pushed a commit to autodesk-forks/USD that referenced this pull request Apr 16, 2024

Fix Windows/Linux build break after IOS feature is merged. (PixarAnim…

9c2e17a

…ationStudios#214)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[USD] Adding position interpolation to points. #214

[USD] Adding position interpolation to points. #214

sirpalee commented May 24, 2017

spiffmon left a comment

spiffmon May 25, 2017

sirpalee May 26, 2017

spiffmon May 25, 2017

sirpalee May 26, 2017

spiffmon May 25, 2017

sirpalee May 26, 2017

spiffmon May 25, 2017

spiffmon May 25, 2017

sirpalee May 26, 2017

spiffmon May 25, 2017

sirpalee May 26, 2017

spiffmon May 28, 2017

sirpalee Jun 5, 2017

jtran56 commented Jun 2, 2017

spiffmon commented Jun 5, 2017 via email

spiffmon commented Jun 18, 2017

sirpalee commented Jun 19, 2017

spiffmon commented Jun 19, 2017

sirpalee commented Jun 22, 2017

spiffmon commented Jun 25, 2017 via email

sirpalee commented Jul 25, 2017

[USD] Adding position interpolation to points. #214

[USD] Adding position interpolation to points. #214

Conversation

sirpalee commented May 24, 2017

Description of Change(s)

spiffmon 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

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

jtran56 commented Jun 2, 2017

spiffmon commented Jun 5, 2017 via email

spiffmon commented Jun 18, 2017

sirpalee commented Jun 19, 2017

spiffmon commented Jun 19, 2017

sirpalee commented Jun 22, 2017

spiffmon commented Jun 25, 2017 via email

sirpalee commented Jul 25, 2017