Enable facet drag dynamic effector to link to state effectors by DavilaDawg · Pull Request #1289 · AVSLab/basilisk

DavilaDawg · 2026-02-25T20:13:30Z

Tickets addressed: bsk-1151
Review: By commit
Merge strategy: Merge (no squash)

Description

The purpose of this PR is to augment the facet drag dynamic effector to be able to link to state effectors instead of just being able to link directly with the hub. To do this I added setter and getter functions for the states of the state effectors and updated the function that calculates drag based on the states of its parent so that if the parent is the hub it uses the hub states and if it is a state effector is uses the state effector's states. One commit contains the changes to the header, the next contains changes to the c+, the next contains changes to the integrated test and the last contains documentation changes.

Verification

I added to the integrated test: test_effectorBranching_integrated.py. I also wrote an aerobrake scenario and which compared two identical spacecraft one with facet drag with the hub as the parent and one where facet drag is also attached to 1DOF HRBs and 2DOF HRB's and the resulting dynamics make sense. The unit tests also pass.

Documentation

I added comments to the header file and c++ as well as added a column to the abilities table in bskPrinciples-11.rst. Also added to bskReleaseNotes.rst

Future work

Can use this change for a more accurate aerobraking maneuver scenario.

ReeceHumphreys · 2026-03-02T16:12:57Z

Can we update this PR to have a more descriptive title.

schaubh · 2026-03-02T17:27:07Z

Both the PR title and the PR description need to be more informative. The PR description, right now, just tells me some files were edited. It tells me nothing about what this branch is about, the motivation, expected outcome, assumptions, etc.

schaubh · 2026-03-02T17:29:09Z

[P1] Shadowed property-name fields break segment force routing for 2-DOF parents

FacetDragDynamicEffector re-declares propName_inertialPosition / ...Velocity / ...Attitude in the derived class. Setters update the derived fields, but spinningBodyTwoDOFStateEffector compares through DynamicEffector* and reads the base getter value. This can prevent facet-drag force/torque from being attributed to either segment.

Refs:

src/simulation/dynamics/facetDragEffector/facetDragDynamicEffector.h#L83
src/simulation/dynamics/facetDragEffector/facetDragDynamicEffector.cpp#L131
src/simulation/dynamics/_GeneralModuleFiles/dynamicEffector.h#L90
src/simulation/dynamics/spinningBodies/spinningBodiesTwoDOF/spinningBodyTwoDOFStateEffector.cpp#L344

schaubh · 2026-03-02T17:29:18Z

[P1] Shadowed state-name fields ignore assigned hub state names

FacetDragDynamicEffector also re-declares stateNameOfSigma / stateNameOfVelocity and defaults them to "hubSigma" / "hubVelocity". Spacecraft assignment writes base-class names, but linkInStates() reads the derived shadowed values. With non-default hub naming, getStateObject() can return nullptr, then updateDragDir() dereferences it.

Refs:

src/simulation/dynamics/facetDragEffector/facetDragDynamicEffector.h#L79
src/simulation/dynamics/facetDragEffector/facetDragDynamicEffector.cpp#L43
src/simulation/dynamics/facetDragEffector/facetDragDynamicEffector.cpp#L113
src/simulation/dynamics/spacecraft/spacecraft.h#L99
src/simulation/dynamics/_GeneralModuleFiles/dynParamManager.cpp#L71
src/simulation/dynamics/facetDragEffector/facetDragDynamicEffector.cpp#L181

schaubh · 2026-03-02T17:29:23Z

[P3] New numeric literals violate repo unit-comment rule

AGENTS requires inline unit comments for physically meaningful numeric literals in modified code. New literals were added without unit annotations.

Refs:

src/simulation/dynamics/_GeneralModuleFiles/_UnitTestDynamics/test_effectorBranching_integrated.py#L292
src/simulation/dynamics/_GeneralModuleFiles/_UnitTestDynamics/test_effectorBranching_integrated.py#L583
src/simulation/dynamics/_GeneralModuleFiles/_UnitTestDynamics/test_effectorBranching_integrated.py#L584
src/simulation/dynamics/_GeneralModuleFiles/_UnitTestDynamics/test_effectorBranching_integrated.py#L585

schaubh

I did a rebase and fixed how release notes were added. In the future, when making a PR, please ensure your branch is already rebased on develop. Also, please make the PR title and description section more informative.

Overall getting close. Thanks for the support!
I'm waiting for @andrewmorell feedback on this PR as well.

schaubh · 2026-03-02T17:31:25Z

src/simulation/dynamics/facetDragEffector/facetDragDynamicEffector.h

    Eigen::Vector3d v_hat_B;                        //!< class variable
    BSKLogger bskLogger;                            //!< -- BSK Logging

+    // State names (for hub attachment)


Shouldn't these variables be private variables that are accessed through setters and getters?

Yes they should be. I fixed this.

schaubh · 2026-03-02T17:32:28Z

src/simulation/dynamics/facetDragEffector/facetDragDynamicEffector.cpp

+   this->inertialAttitudeProperty = nullptr;
+
+   // Set default state names for hub attachment
+   this->stateNameOfSigma = "hubSigma";


what about the default name for the position states?

Do you actually use inertialPositionProperty?

I removed this because it was unused.

schaubh · 2026-03-02T17:36:14Z

src/simulation/dynamics/facetDragEffector/facetDragDynamicEffector.cpp

+   this->hubVelocity = nullptr;
+
+   // Initialize property pointers
+   this->inertialPositionProperty = nullptr;


I'm not sure you actually use this variable?

I don't, I removed it.

DavilaDawg · 2026-03-03T00:59:59Z

I have removed the unnecessary variables and also removed the getters as they are inherited from DynamicEffector. I kept the setter functions with an override but I could remove those too if needed.

DavilaDawg requested review from andrewmorell and schaubh February 25, 2026 20:13

DavilaDawg requested a review from a team as a code owner February 25, 2026 20:13

DavilaDawg added this to Basilisk Feb 25, 2026

DavilaDawg added the enhancement New feature or request label Feb 25, 2026

schaubh assigned DavilaDawg Feb 28, 2026

schaubh moved this to 👀 In review in Basilisk Feb 28, 2026

schaubh linked an issue Feb 28, 2026 that may be closed by this pull request

Hinged 2DOF branching #1151

Open

schaubh force-pushed the feature/facetDrag-branching branch from f0adf7c to 8a29fe9 Compare March 2, 2026 15:39

schaubh requested changes Mar 2, 2026

View reviewed changes

DavilaDawg changed the title ~~Feature/facet drag branching~~ Enable Facet drag to link to state effectors Mar 2, 2026

DavilaDawg changed the title ~~Enable Facet drag to link to state effectors~~ Enable facet drag dynamic effector to link to state effectors Mar 2, 2026

DavilaDawg force-pushed the feature/facetDrag-branching branch from f584912 to 4132d69 Compare March 3, 2026 00:53

DavilaDawg added 4 commits March 2, 2026 16:56

header file for facet drag branching

1c2e1bc

write methods for state effector branching

8ee2ff9

add facet drag as dynamic effector in test

3959728

documentation and release notes

af9e445

DavilaDawg force-pushed the feature/facetDrag-branching branch from 4132d69 to af9e445 Compare March 3, 2026 00:57

Conversation

DavilaDawg commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Verification

Documentation

Future work

Uh oh!

ReeceHumphreys commented Mar 2, 2026

Uh oh!

schaubh commented Mar 2, 2026

Uh oh!

schaubh commented Mar 2, 2026

[P1] Shadowed property-name fields break segment force routing for 2-DOF parents

Uh oh!

schaubh commented Mar 2, 2026

[P1] Shadowed state-name fields ignore assigned hub state names

Uh oh!

schaubh commented Mar 2, 2026

[P3] New numeric literals violate repo unit-comment rule

Uh oh!

schaubh left a comment

Choose a reason for hiding this comment

Uh oh!

schaubh Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

DavilaDawg Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

schaubh Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

schaubh Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

DavilaDawg Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

schaubh Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

DavilaDawg Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

DavilaDawg commented Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DavilaDawg commented Feb 25, 2026 •

edited

Loading