Skip to content

Feature/979 mujoco stateful sysmodel#980

Merged
juan-g-bonilla merged 8 commits into
developfrom
feature/979-mujoco-stateful-sysmodel
Apr 18, 2025
Merged

Feature/979 mujoco stateful sysmodel#980
juan-g-bonilla merged 8 commits into
developfrom
feature/979-mujoco-stateful-sysmodel

Conversation

@juan-g-bonilla
Copy link
Copy Markdown
Contributor

@juan-g-bonilla juan-g-bonilla commented Apr 14, 2025

Description

Currently, the MJScene dynamic object cannot have continuous states, other than the states of the multi-body. A continuous state is a state whose evolution is given by its time derivative, and thus must be integrated forward in time. spacecraft objects use stateEffector to declare such states (for example, a fuel tank is modeled as a stateEffector whose mass in a continuous state). A similar capability is required for MJScene.

To address this, a new class, StatefulSysModel, has been added, which inherits from SysModel but has an additional virtual method registerStates. MJScene has an internal "dynamics" task, which contains SysModel objects that compute the forces/torques applied on the multi-body system. With StatefulSysModel, we make it so that the SysModel objects in the "dynamics" task can also declare their own continuous-time states and then compute their derivative on their UpdateState method.

StatefulSysModel have a new method, registerStates, which is called on the initializeDynamics function in MJScene (the same method where the mujoco multi-body declares its states or stateEffector registered their states in spacecraft). The main difference with stateEffector is that StatefulSysModel get a DynParamRegisterer objects, instead of a DynParamManager. DynParamRegisterer wraps an instance of DynParamManager, but allows only one function to be called on it: registerState. Moreover, it prepends some model-unique string to the state name before registering it on the manager, which prevents state name collision between models (multiple models can register the same state name).

DynParamRegisterer purposefully grants access to only one method registerState, because I consider other methods in the manager to be transparent data sharing, which can be harmful to understanding the flow of data in the simulation. Shared state objects and properties allow models to talk to each other and influence each other through a parallel information passing scheme to messages. The message system provides clear model interfaces, functional encapsulation, and ease of testing and logging. Data sharing through states and properties goes against this. If a model wants to share its state information, I believe it should do so through a message. If a model needs to know a "property" of the multi-body (such as its center of mass or gravity acceleration), then a model should compute and properly publish this information as a message. With this new pattern, state objects are managed by a single model, and properties are deprecated.

Moreover, Python StatefulSysModel are supported. With this feature, we are able to effectively replicate the dynamicEffector and stateEffector functionality purely on the Python side, which will enable rapid prototyping.

Verification

A new test src/simulation/mujocoDynamics/_GeneralModuleFiles/_UnitTest/test_stateful_sys_model.py has been written. Moreover, a scenario examples/mujoco/scenarioDeployPanels.py has been updated to show the use of StatefulSysModel (in Python).

Documentation

No documentation has been added, other than the showcase in scenarioDeployPanels.py. Release notes have not been updated, since the previous MuJoCo entry covers this content.

@juan-g-bonilla juan-g-bonilla added the enhancement New feature or request label Apr 14, 2025
@juan-g-bonilla juan-g-bonilla self-assigned this Apr 14, 2025
@juan-g-bonilla juan-g-bonilla requested a review from a team as a code owner April 14, 2025 07:33
@juan-g-bonilla juan-g-bonilla force-pushed the feature/979-mujoco-stateful-sysmodel branch from 8ed72f7 to 8eebd8f Compare April 14, 2025 15:26
@juan-g-bonilla juan-g-bonilla moved this to 👀 In review in Basilisk Apr 14, 2025
@schaubh schaubh self-requested a review April 15, 2025 13:52
Copy link
Copy Markdown
Contributor

@schaubh schaubh left a comment

Choose a reason for hiding this comment

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

Did a quick skim across this change. Would like to Zoom with you to discuss in more detail to follow the motivation here. In the PR description, I would prefer not to call other work "evil". If this new approach has benefits, then let's be clear about the benefits of this approach and not insult prior efforts.

Also, the PR text gives a clear description on what is happening, but it is unclear to the reviewer why this change is being made. I would prefer to start such a discussion with a brief paragraph talking about the motivation for this work. I'm assuming this is going to enable you to integrate other stateEffectors in the future where the force/torque info is pulled from the output messages? You outlined to me last December your concept of doing the RK4, etc. integration only using messages and not having these specific function calls on the effectors?

Comment thread src/simulation/mujocoDynamics/_GeneralModuleFiles/StatefulSysModel.h Outdated
@juan-g-bonilla
Copy link
Copy Markdown
Contributor Author

Did a quick skim across this change. Would like to Zoom with you to discuss in more detail to follow the motivation here. In the PR description, I would prefer not to call other work "evil". If this new approach has benefits, then let's be clear about the benefits of this approach and not insult prior efforts.

Also, the PR text gives a clear description on what is happening, but it is unclear to the reviewer why this change is being made. I would prefer to start such a discussion with a brief paragraph talking about the motivation for this work. I'm assuming this is going to enable you to integrate other stateEffectors in the future where the force/torque info is pulled from the output messages? You outlined to me last December your concept of doing the RK4, etc. integration only using messages and not having these specific function calls on the effectors?

Really didn't mean to be insulting to other work, I used "evil" in the exaggerated trying-to-be-funny way programmers talk about things they don't agree with.

There's a bit more context/explanation for this feature on the associated issue, I'll put that explanation here too. I'll also put 30 mins on the calendar to chat about this!

Copy link
Copy Markdown
Contributor

@schaubh schaubh left a comment

Choose a reason for hiding this comment

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

add release notes discussing the MuJoCo scenario having been changed.

This allows us to %import this file on other .i files
@juan-g-bonilla juan-g-bonilla force-pushed the feature/979-mujoco-stateful-sysmodel branch from c79d0a2 to 6a69bba Compare April 18, 2025 01:03
@juan-g-bonilla juan-g-bonilla force-pushed the feature/979-mujoco-stateful-sysmodel branch from 82078f4 to 8f775c8 Compare April 18, 2025 01:06
@juan-g-bonilla juan-g-bonilla merged commit 3572f8b into develop Apr 18, 2025
11 checks passed
@juan-g-bonilla juan-g-bonilla deleted the feature/979-mujoco-stateful-sysmodel branch April 18, 2025 04:25
@github-project-automation github-project-automation Bot moved this from 👀 In review to ✅ Done in Basilisk Apr 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

Add stateful SysModel for mujoco Dynamics

2 participants