Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FPS metadata to exported usd files if non static #913

Merged
merged 1 commit into from
Nov 13, 2020

Conversation

dgovil
Copy link
Collaborator

@dgovil dgovil commented Nov 11, 2020

The current exporter behavior writes out frames as if the USD is a the current scenes uiUnits, but does not actually set the fps attributes in the exported layer. This causes downstream readers to treat it as 24fps as is default.

This change adds both TimeCodesPerSecond and FramesPerSecond, if the exporter is exporting animation. This will allow downstream readers to playback at the correct timing.

As a side note...it is surprising that MTimeUnit doesn't have an API method to convert to float values. Would be great in the future if that was exposed, rather than resorting to a hardcoded mapping here.

@kxl-adsk kxl-adsk requested a review from mattyjams November 12, 2020 01:56
@kxl-adsk
Copy link

I glance over the changes and notice a change in formatting - we have configured clang-format for maya-usd repository and pull-request authors are required to apply clang-format (using version 10.0.0) to the modified files. Can you please make sure to do it for all your open PRs?

@kxl-adsk kxl-adsk added the import-export Related to Import and/or Export label Nov 12, 2020
@dgovil
Copy link
Collaborator Author

dgovil commented Nov 12, 2020

@kxl-adsk Are you seeing any formatting changes in particular?

I'm not sure I see the formatting issues after running clang-format manually again (did this for all the PRs). I ran clang-format -i on the modified .cpp and .h files, and the files were identical. Additionally I have clang format running as part of my CLion setup and it's not detecting any style infractions.

Also tried running the command specified in your PR find -E . -regex "$(echo '.*('$(echo $(cat .clang-format-include) | tr ' ' '\|')')')" -exec clang-format -i {} \+ , however that modified a bunch of files that are not part of any of my PRs:
formatting.patch.txt

        modified:   lib/mayaUsd/fileio/transformWriter.cpp
        modified:   lib/mayaUsd/render/vp2RenderDelegate/proxyRenderDelegate.cpp
        modified:   lib/mayaUsd/render/vp2RenderDelegate/render_delegate.h
        modified:   lib/mayaUsd/render/vp2RenderDelegate/render_param.h
        modified:   lib/mayaUsd/ufe/UsdAttributes.cpp
        modified:   lib/mayaUsd/ufe/UsdRotatePivotTranslateUndoableCommand.h
        modified:   lib/mayaUsd/ufe/UsdRotateUndoableCommand.h
        modified:   lib/mayaUsd/ufe/UsdScaleUndoableCommand.h
        modified:   lib/mayaUsd/ufe/UsdTranslateUndoableCommand.h
        modified:   lib/usd/translators/nurbsSurfaceWriter.cpp
        modified:   lib/usd/utils/SIMD.h
        modified:   plugin/al/lib/AL_USDMaya/AL/usdmaya/Global.h
        modified:   plugin/al/lib/AL_USDMaya/AL/usdmaya/nodes/ProxyShape.h
        modified:   plugin/al/mayautils/AL/maya/event/MayaEventManager.cpp
        modified:   plugin/al/mayautils/AL/maya/event/MayaEventManager.h
        modified:   plugin/al/mayautils/AL/maya/utils/FileTranslatorOptions.cpp
        modified:   plugin/al/mayautils/AL/maya/utils/FileTranslatorOptions.h
        modified:   plugin/al/translators/Mesh.cpp
        modified:   plugin/pxr/maya/lib/usdMaya/referenceAssembly.cpp

For reference, I'm using your clang format file from bb0704a
My clang-format version is 11.0.0 on a Mac. Is it possible that our clang formats are configured differently perhaps?

@kxl-adsk
Copy link

It's the change in utils.h where I was expecting indentation - https://github.com/Autodesk/maya-usd/pull/913/files#diff-68214d17766b3d5a5678b676f7939a4d4899e99d1f5152a8a8c0e24e933d7078R575

But the more important is the version of clang-format. You mentioned version 11.0.0 but we ask developers to all use version 10.0.0. This is because different versions produce different results for the same clang-format configuration file.

@dgovil
Copy link
Collaborator Author

dgovil commented Nov 12, 2020

I tried formatting the files again with clang-format 10 (downloaded from the link in #890 since homebrew no longer has a recipe for 10 available) and it's still not marking these files as needing changes. Running it across the entirety of the repo did show a different set of modified files than clang-format 11 so that does show the versions vary quite a bit.

Just to confirm though, if you run clang-format on this branch, does it change utils.h? I just want to verify if it's an issue in the config versus something on our end here. Visually the style seems to match the other function signatures higher in the file to me.

Here are the results of running clang-format 10 across the whole repo on my end:

formatting.patch.txt

	modified:   lib/mayaUsd/ufe/UsdAttributes.cpp
	modified:   plugin/al/mayautils/AL/maya/event/MayaEventManager.cpp
	modified:   plugin/al/mayautils/AL/maya/event/MayaEventManager.h
	modified:   plugin/al/mayautils/AL/maya/utils/FileTranslatorOptions.cpp

@kxl-adsk
Copy link

kxl-adsk commented Nov 12, 2020

Re utils.h - you are right, I must have looked at it too quickly.

Re clang-format version and what's currently still showing up after running it. Indeed, we still have 4 files that are now showing a difference after you run clang-format v10.0.0 on the entire repo. We have discovered just recently that in some cases, clang-format may have to be applied more than once. I will open a PR for the remaining files.

@dgovil
Copy link
Collaborator Author

dgovil commented Nov 12, 2020

Ok great to know it's not some deeper issue. I'll move around the unittests per the other PR and then update this one.

@dgovil
Copy link
Collaborator Author

dgovil commented Nov 12, 2020

Okay, updated the test location. Hopefully should be good to go, but let me know if you see anything else of concern. I'll update the other PRs shortly too.

Copy link
Contributor

@mattyjams mattyjams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some small cosmetic suggestions, but otherwise looks good to me! Thanks @dgovil!

@kxl-adsk kxl-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Nov 13, 2020
@kxl-adsk kxl-adsk merged commit f670d16 into Autodesk:dev Nov 13, 2020
@dgovil dgovil deleted the fps_metadata branch November 20, 2020 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
import-export Related to Import and/or Export ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants