Convert USD timeSamples to Maya FPS #970
Conversation
|
@dgovil Apologies for taking long to get to your pull requests. We have some internal deadlines that affected our ability to get to this PR. |
|
@kxl-adsk best of luck with that, that's understandable. |
|
@dgovil I was looking for someone who could help us review this change. From one side, it looks pretty straightforward...but remembering all the trickery with supporting different time codes, like 29.976, I didn't want to overlook something that would cause us to affect precision or roundtripping of data. I asked @langlor-autodesk to take a look and at the very least ask questions that will confirm that we are not ignoring anything important here. |
|
Thanks! yes, i would appreciate the double checking on precision losses. Should be easy to modify the tests accordingly too |
There was a problem hiding this comment.
I'll leave it to @fowlertADSK and @langlor-autodesk for the final approval, but it generally makes sense to me.
I left a bunch of comments, but they're all around the same request: It would be good not to require a prim reader context in the various translator classes. At some point, we want to have better support for ad-hoc translation outside the context of an import. That was the aspiration initially, though it's probably not a clean enough API for that purpose at the moment.
|
Thanks @mattyjams , and thanks for highlighting all the uses. Did a quick Just rebuilt and updated the commit. |
Sweet, thanks @dgovil! :) |
|
I don't know for USD but Maya internally handles times using a fixed point representation which ensure a constant precision across the entire time domain. However MTime only exposes these times using floating points. These floating points are more precise than Maya internal time close to time 0 but they become less precise than Maya for large time values. This difference of precision could affect round trip workflows if the Maya USD plugin supports such workflows. When testing, please ensure tests cover non integer framerates and fractional keyframe values. |
lib/mayaUsd/fileio/jobs/readJob.cpp
Outdated
|
|
||
| double UsdMaya_ReadJob::_setTimeSampleMultiplierFrom(const double layerFPS) | ||
| { | ||
| float sceneFPS = UsdMayaUtil::GetSceneMTimeUnitAsFloat(); |
There was a problem hiding this comment.
Do you have a more precise version of that method? (I dont know if GetSceneMTimeUnitAsDouble exists). For some non integer framerates, like Ntsc 59.94fps which is equal to 60000/1001 = 59.94005994005994fps, the representation is not the same in float as in double which will result in imprecision when this fps will be used to compute frame values. Example, Frame 1000 when translated from 60fps to 59.94fps will give 1001.0000046458563 instead of the expected 1001 value
There was a problem hiding this comment.
This function is only used in one other place for USD metadata so I can change it to double instead of float.
Is there a list of constant values and/or mathematical formulae to use for correctness? Alternatively I can just update this PR with the changes to double tomorrow and if you can let me know which values should be modified, that would be awesome.
Otherwise for drop frames I'll just use the nearest whole frame*1000/1001
E.g 24000/1001, 30000/1001, 48000/1001, 60000/10001
Semi related, it would be awesome if a future API version of MTime provided these out of the box to prevent discrepancies in different implementations that each provide their own mapping.
There was a problem hiding this comment.
The only non integer framerates that Maya currently supports (24df, 30df, 48df and 60fc) are all based on the same multiplier 1000/1001. So for 24df = 241000/1001 ~= 23.976, 30df ~= 29.97 and 60df=601000/1001 ~= 59.94
Unless USD does not support the notion of partial frame, rounding can usually and should usually be avoided.
There was a problem hiding this comment.
@langlor-autodesk Thanks, I updated the PR with a change to util.cpp/h to change it all to doubles.
I also changed all the drop frame conversions to wholeFrame * 1000.0 / 1001.
Hopefully everything looks good now? I was unsure as to the difference between k29_97FPS and k29_97DF however, so used the same math for both. I hope that's correct.
|
@dgovil Can you please address clang format errors. |
Include higher precision calculation for drop frame time units
|
@kxl-adsk Done, apologies, didn't think it would want to unindent those lines with the latest changes. |
| } break; | ||
| case MTime::k59_94FPS: { | ||
| ret = 59.94f; | ||
| ret = (60.0 * 1000.0) / 100; |
There was a problem hiding this comment.
Hey @dgovil! Sorry, only just noticing this as I'm integrating these changes internally.
I think 1001.0 was probably what was intended here rather than 100?
There was a problem hiding this comment.
oh yes! I must have hit backspace. let me put up a new PR
Hi,
This PR adds support for conversion on import from the USD's timeCodesPerUnits to the Maya scenes uiUnit values.
Since USD technically supports arbitrary fps, I do not try and map from their FPS values to one of MTime units, and instead calculate a TimeSampleMultiplier value.
I believe I've caught all the uses of MTimeArray and MTime, and my tests cover most object types that I believe are supported, with the exception of NurbsCurves since it doesn't seem like the importer actually imports them? At least I couldn't coerce an import.