Skip to content

Add eclipse illumination support to facetSRPDynamicEffector#1336

Open
themouli wants to merge 3 commits intoAVSLab:developfrom
themouli:feature/bsk-1335--facet-srp-eclipse
Open

Add eclipse illumination support to facetSRPDynamicEffector#1336
themouli wants to merge 3 commits intoAVSLab:developfrom
themouli:feature/bsk-1335--facet-srp-eclipse

Conversation

@themouli
Copy link

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

Description

Added an optional sunEclipseInMsg input to FacetSRPDynamicEffector so that eclipse shadow conditions are accounted for in the SRP force and torque calculation. When connected, the computed SRP force and torque are scaled by the shadow factor from the EclipseMsgPayload (0.0 = full shadow, 1.0 = full sunlight). When not connected, behavior is unchanged (defaults to full sunlight).

Verification

Updated the existing unit test with a new eclipseShadowFactor parametrization that creates and subscribes an EclipseMsg and scales the Python truth values accordingly.

Documentation

Updated facetSRPDynamicEffector.rst:

  • Added sunEclipseInMsg to the Module I/O Messages table.
  • Added a user guide step showing how to connect the eclipse message.

@themouli themouli requested a review from a team as a code owner March 20, 2026 07:33
@themouli themouli changed the title [#1335] Add eclipse shadow support to facetSRPDynamicEffector PR for issue BSK-1335: Add eclipse shadow support to facetSRPDynamicEffector Mar 20, 2026
@schaubh schaubh self-requested a review March 20, 2026 13:42
@schaubh
Copy link
Contributor

schaubh commented Mar 20, 2026

@codex, do a fun review of this PR using AGENTS.md

@schaubh schaubh self-assigned this Mar 20, 2026
@schaubh schaubh added documentation Improvements or additions to documentation enhancement New feature or request labels Mar 20, 2026
@schaubh schaubh added this to Basilisk Mar 20, 2026
@schaubh schaubh linked an issue Mar 20, 2026 that may be closed by this pull request
@schaubh schaubh moved this to 👀 In review in Basilisk Mar 20, 2026
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4b4f91e446

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

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.

Need to add info to release notes. We now do this by adding a small release notes RST snippet to docs/source/Support/bskReleaseNotesSnippets. For example, this file could contain

- added support for eclipse illumination factor to :ref:`facetSRPDynamicEffector`

@schaubh
Copy link
Contributor

schaubh commented Mar 20, 2026

Also, please be sure to rebase, not merge, this branch on the latest code in develop.

@themouli themouli force-pushed the feature/bsk-1335--facet-srp-eclipse branch from 53d912a to f58d2ef Compare March 20, 2026 14:20
@themouli
Copy link
Author

Address all the comments.

  • Updated to illuminationFactor instead of depreciated shadowFactor
  • Added release notes
  • Zero-initialized sunVisibilityFactor{} in the header
  • Added default illumination factor before if statement

Ready for re-review. Thanks for thorough feedback @schaubh!

@themouli themouli force-pushed the feature/bsk-1335--facet-srp-eclipse branch from f58d2ef to 0eea2a5 Compare March 20, 2026 14:37
@themouli
Copy link
Author

Cleaned the commit history

@schaubh schaubh changed the title PR for issue BSK-1335: Add eclipse shadow support to facetSRPDynamicEffector Add eclipse illumination support to facetSRPDynamicEffector Mar 20, 2026
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.

Just minor renaming. I caught more instances of "shadow factor". Should be a quick fix. I can then close this after these changes.

@themouli themouli force-pushed the feature/bsk-1335--facet-srp-eclipse branch from 0eea2a5 to 8b97e09 Compare March 21, 2026 01:45
@themouli
Copy link
Author

I missed it. Updated them. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request

Projects

Status: 👀 In review

Development

Successfully merging this pull request may close these issues.

facetSRPDynamicEffector does not account for eclipse shadow

2 participants