Skip to content

Conversation

joaogvcarneiro
Copy link
Contributor

@joaogvcarneiro joaogvcarneiro commented Sep 26, 2025

Description

The commits first run through spinningBodiesNDOF, and then move onto linearTranslationBodiesNDOF. First, all SpinningBody and TranslatingBody structures are made shared_ptr. Then, the .i files are changed appropriately to accommodate the use of shared pointers. Finally, a getter method is added for each body so that they can be changed in Python.

Verification

All tests pass as expected.

Documentation

N/A.

Future work

N/A.

@joaogvcarneiro joaogvcarneiro requested a review from a team as a code owner September 26, 2025 21:34
@joaogvcarneiro joaogvcarneiro changed the title Feature/expose individual bodies n dof effector Expose individual bodies NDOF effector Sep 26, 2025
@joaogvcarneiro joaogvcarneiro marked this pull request as draft September 26, 2025 21:36
Copy link
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.

Nice PR. Thanks for upgrading both effectors. Some todos:

  • need to deprecate the use of the all effector class name
  • I don't see any unit tests checking that we can change values on the fly. We should test this in a unit test for both effectors.

#include "architecture/messaging/messaging.h"

/*! @brief linear spring mass damper state effector class */
class linearTranslationOneDOFStateEffector :
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to break existing code. I agree with the fix, but can we deprecate this class name fix? Could we use

class MyClass {
    // ...
};

using OtherName = MyClass;

OtherName x;   // same as MyClass

to achieve this and keep backwards compatibility for a year?

Next question, can we give a deprecation warning if someone uses the old name?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants