Add NewtonSceneAPI

[andrewkaufman]

Description

This is an initial implementation of #2 to test out the CI workflow now that we are public.

Checklist

• I am familiar with the Contributing Guidelines.

[codecov]

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment

Thanks for integrating Codecov - We've got you covered ☂️

[andrewkaufman]

I pushed an update to limit NewtonSceneAPI to PhysicsScene prims only & I added an API level doc string.

I decided to leave the other attribute strings simple for now, we can revisit if someone has a better suggestion in review.

[vastsoun]

LGTM overall.

My only comment pertains to the docstring of the max iterations parameter.

[andrewkaufman]

@vastsoun I addressed your comment & also pushed an additional change to add enableGravity as we discussed in the meeting. Please re-review 🙇

[AlesBorovicka]

Can we please add a convention to all the attributes related to ranges and units? Like this ideally:
Range: [0, inf)
Units: dimensionless
It would make our live easier for validation, we can process the ranges and units and validate. Ideally we follow some convention. The above is what we already use.
See also: https://github.com/PixarAnimationStudios/OpenUSD/blob/release/pxr/usd/usdPhysics/schema.usda#L576

[AlesBorovicka]

Before we go double crazy, can we double check with Pixar? When we wrote UsdPhysics it was recommended to use floats everywhere instead of doubles. Using double in timeStep is strange, if you care about precision why do we not use integer and frequency.

[andrewkaufman]

Yep I will double check. Happy to change to float

[andrewkaufman]

It seems they are both used frequently. Interesting note from Claude, that kind of implies UsdPhysics should use more doubles than it does currently:

Semantic distinction:
float → rendering/visual parameters, light properties, camera settings
double → geometric dimensions, precise measurements, parametric ranges

On the timeStep, I agree with @AlesBorovicka point that float is good enough for timeStep, so I've changed that. If anyone disagrees please re-open the thread.

[andrewkaufman]

Though probably worth pointing out this change required switching to assertAlmostEqual in the unittests, so we immediately hit the precision issue with the fallback value 0.005

[AlesBorovicka]

But if you care about precision why dont use hz - uint? Even authoring these tiny values dont make sense. Hz are very clear, any engine can divide...

[AlesBorovicka]

We do use it in PhysxSchema, it seems to be supported by USD.

Yes, I would suggest we discuss this next time, hope I wont get a conflict meeting this time.

[andrewkaufman]

uint is a supported type for sure, it just doesn't occur in any schema.usda in the USD repo (fakes schemas for test coverage aside). All of the "should probably be a uint" attributes in USD official schemas use int instead (e.g. sampling settings, indices)

[preist-nvidia]

Using frequency makes sense to me too, and it's what we already have in the PhysxSceneAPI: https://docs.omniverse.nvidia.com/kit/docs/omni_physics/latest/dev_guide/schemas/physxschema.html#_CPPv4NK24PhysxSchemaPhysxSceneAPI25GetTimeStepsPerSecondAttrEv

[andrewkaufman]

FWIW MJC uses double

https://github.com/google-deepmind/mujoco/blob/main/src/experimental/usd/mjcPhysics/generatedSchema.usda#L266-L269

https://mujoco.readthedocs.io/en/stable/XMLreference.html#option-timestep

[vastsoun]

FWIW, I would also be OK with frequency.

The rationale makes sense as it is closer to thinking in terms of frame-rate, which often is more intuitive for many users.

[andrewkaufman]

Can we please add a convention to all the attributes related to ranges and units? Like this ideally:
Range: [0, inf)
Units: dimensionless

There is a fairly new official mechanism for range UI hints, I think we should adopt the official system instead. I've split this out into #11 to investigate the new mechanism. We can still do it asap, just don't want to block this PR on it.

[Milad-Rakhsha-NV]

is 100 a reasonable default value? I think a reasonable default value is way smaller for both mjc and physX IIRC.
I think we should either interpolate between the default values used by physX and MJC, or just use of them directly

[andrewkaufman]

You had 100 in the spreadsheet, so I assumed that's what people agreed to. Happy to change it to any value that has majority consensus.

[andrewkaufman]

MJC is 100

https://github.com/google-deepmind/mujoco/blob/main/src/experimental/usd/mjcPhysics/generatedSchema.usda#L201-L204

[Milad-Rakhsha-NV]

yeah I must have taken the mujoco default but for physX articulations for instance we have the following defaults
physxArticulation:solverPositionIterationCount = 32
physxArticulation:solverVelocityIterationCount = 1

the global ones (which I am not sure how are used in conjunction with per-actor ones above
physxScene:maxVelocityIterationCount = 255
physxScene:maxPositionIterationCount = 255

XPBD default iteration count is 2 for cloth solvers like VBD/Style3D this is 10.

[Milad-Rakhsha-NV]

I am having second thoughts on this and I am wondering if we should add this at all to scene given that this is solver parameter. For instance, I imagine that specific solvers may want to avoid using this (in case this is also used for any other subsystem they don't want to mess up, e.g. having a both cloth solver and rigid solver in the scene, the cloth solver may want to use different value)

[Milad-Rakhsha-NV]

Let's discuss the options in the next meeting. The other option is to have each solver define their own schema API and add it to the single scene prim, but I agree that timestep and iteration are two of the most common solver parameters that probably make sense to have here. @AlesBorovicka @preist-nvidia what do you think?

[AlesBorovicka]

I always thought we would use variants for this or different scenes, if there is a clear separation in the stage that some part is simulated by one engine and another by another engine, then you should have two scenes, hence two APIs that define this.
If this is within one asset I though we use variants for the engines or as proposed by Renato sublayers, though I have to say I would prefer variants to have all self contained in one asset. But I expect that it will happen that some parameters will be different for a different engine especially parameters that dont have physical meaning like the solve iteration count, step size...

[vastsoun]

1. Regarding the question of default values:
   Does it make sense to use sentinel values (e.g. -1) by default to allow solvers to configure themselves according to their own defaults, that they set/define internally? This would avoid the question altogether, and then just allow the resolver/importer to set values only if set explicitly.

2. On the matter of how some solvers actually require more than one maxiters, or a very special set:
   Why not treat this as scene attribute as optional, just like how in other parts of USD, e.g. not all physics backends realize certain types of friction models like static/dynamics, so it's up to the implementation to decide whether to use it or not. So in effect, I'm propose to have newton:solverMaxIterations, but use it only if-and-only-if it's set and supported.

As others mentioned above, if a solver requires it's own special configs, then their custom API could be the mechanism to facilitate it.

[andrewkaufman]

I like the sentinal idea & also it justifies int over uint 😅

[Milad-Rakhsha-NV]

yeah that sounds good, I am fine with keeping them since they are super generic and any solver can use them really with their own interpretation of what that should mean

[havess]

LGTM, I'm not super pressed about maxSolverIterations defaults. Odds are if you're authoring this you know what you're targeting, and defaults are easy enough to change later.

[vastsoun]

@andrewkaufman I've provided a few comments, and like the other reviewers, regarding the question of SceneAPI parameters and default values.

One other concern I have is regrading the default values used for gravity, as internationally the globally accepted value is actually 9.80665 m⋅s−2, and not 9.81. This is more of a question for Newton sim, but I raise it here, as to whether the NewtonSceneAPI should consider using an actual number instead of the -INF value of USD.

[vastsoun]

1. Regarding the question of default values:
   Does it make sense to use sentinel values (e.g. -1) by default to allow solvers to configure themselves according to their own defaults, that they set/define internally? This would avoid the question altogether, and then just allow the resolver/importer to set values only if set explicitly.

2. On the matter of how some solvers actually require more than one maxiters, or a very special set:
   Why not treat this as scene attribute as optional, just like how in other parts of USD, e.g. not all physics backends realize certain types of friction models like static/dynamics, so it's up to the implementation to decide whether to use it or not. So in effect, I'm propose to have newton:solverMaxIterations, but use it only if-and-only-if it's set and supported.

As others mentioned above, if a solver requires it's own special configs, then their custom API could be the mechanism to facilitate it.

[vastsoun]

IMHO 1ms might be a more robust value for default time-step.

Within the context of in simulation and control, I have the impression 0.001 is considered a gold-standard, so it might pose a better and more sane default value.

It is also the default of Kamino, and was selected as such because of the aforedescribed rationale.

[vastsoun]

Overall LGTM, we can always come back with refinements to defaults at a later time. Probably building up the skeleton and mean is more important than the details.

[AlesBorovicka]

@andrewkaufman I've provided a few comments, and like the other reviewers, regarding the question of SceneAPI parameters and default values.

One other concern I have is regrading the default values used for gravity, as internationally the globally accepted value is actually 9.80665 m⋅s−2, and not 9.81. This is more of a question for Newton sim, but I raise it here, as to whether the NewtonSceneAPI should consider using an actual number instead of the -INF value of USD.

This is deliberate (-INF) and it should stay, you cant have a real value there, because USD by default can be in any units, not just meters. Hence many of the attributes use as a default value a sentinel value, where the value is computed based on the actual units.

[vastsoun]

@andrewkaufman I've provided a few comments, and like the other reviewers, regarding the question of SceneAPI parameters and default values.
One other concern I have is regrading the default values used for gravity, as internationally the globally accepted value is actually 9.80665 m⋅s−2, and not 9.81. This is more of a question for Newton sim, but I raise it here, as to whether the NewtonSceneAPI should consider using an actual number instead of the -INF value of USD.

This is deliberate (-INF) and it should stay, you cant have a real value there, because USD by default can be in any units, not just meters. Hence many of the attributes use as a default value a sentinel value, where the value is computed based on the actual units.

Oh, I wasn't aware. Thanks for the clarification @AlesBorovicka.