feat(SOLSYS-71): Sol → TRAPPIST-1 cinematic - #72
Merged
Conversation
Destination chapter from Sol through the neighborhood to TRAPPIST-1 at true Sol XYZ, finishing on the b–h resonant chain using the #69 spin packs.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a Sol→TRAPPIST-1 cinematic animator with staged travel, seven-planet rendering, optional Blender billboards, CLI support, tests, and updated documentation. ChangesSol→TRAPPIST-1 cinematic
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RenderCLI
participant SolTrappistCinematicAnimator
participant StarCatalog
participant BlenderBodyRegistry
RenderCLI->>SolTrappistCinematicAnimator: select sol_trappist
SolTrappistCinematicAnimator->>StarCatalog: load TRAPPIST-1 data
SolTrappistCinematicAnimator->>StarCatalog: calculate travel and orbital positions
SolTrappistCinematicAnimator->>BlenderBodyRegistry: queue optional planet billboards
SolTrappistCinematicAnimator-->>RenderCLI: write light and dark GIF outputs
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
animate/scenes/sol_trappist_cinematic.py (1)
88-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftParameterize
SolCentauriCinematicAnimatorinstead of copying it into the subclass. Both sites copy base-class code verbatim and change only a few destination-specific values, so future edits to the base class will not reachSolTrappistCinematicAnimator.
animate/scenes/sol_trappist_cinematic.py#L88-L216: replace the copied initializer withsuper().__init__(...)plus overridable hooks for destination geometry and framing constants; this also clears the CodeQL missing-super finding at Line 85.animate/scenes/sol_trappist_cinematic.py#L240-L271: delete this override and add class attributes for the field-star radius and the excludedsystem_idto the base_loadFieldStars.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@animate/scenes/sol_trappist_cinematic.py` around lines 88 - 216, Refactor SolTrappistCinematicAnimator.__init__ (animate/scenes/sol_trappist_cinematic.py#L88-L216) to call SolCentauriCinematicAnimator’s initializer via super().__init__(...), moving destination-specific geometry and framing values into overridable hooks or class attributes while preserving TRAPPIST-1 validation and setup; this also resolves the missing-super finding. Remove the _loadFieldStars override at animate/scenes/sol_trappist_cinematic.py#L240-L271, and add class attributes consumed by the base implementation for the field-star radius and excluded system_id.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_sol_trappist_cinematic.py`:
- Around line 140-143: Update the inner-arrival assertion in the test around
animator._cameraState to evaluate a frame at or after
ARRIVAL_TRAPPIST_INNER_ARRIVE, then assert that innerHalf reaches
TRAPPIST_INNER_HALF_AU rather than only satisfying the wide-framing upper bound.
Preserve the existing focus assertion and Blender timing coverage.
- Around line 101-107: Strengthen test_planets_orbit_host by validating each
planet’s radial distance against its semiMajorAxisAu and eccentricity rather
than only the broad host offset bound. Evaluate _trappistPlanetPositionSol at
two distinct frames and assert the positions differ appropriately, while
retaining the existing planet-count and planar-orbit checks.
- Around line 109-113: Update test_field_stars_reach_trappist_distance to
require a non-empty animator.fieldStars selection, then assert at least one
selected star has a distance in the intended 30–45 ly extended range. Keep the
trappist_1 exclusion assertion unconditional rather than guarding it with an
emptiness check, and retain the existing configuration-bound assertions.
---
Nitpick comments:
In `@animate/scenes/sol_trappist_cinematic.py`:
- Around line 88-216: Refactor SolTrappistCinematicAnimator.__init__
(animate/scenes/sol_trappist_cinematic.py#L88-L216) to call
SolCentauriCinematicAnimator’s initializer via super().__init__(...), moving
destination-specific geometry and framing values into overridable hooks or class
attributes while preserving TRAPPIST-1 validation and setup; this also resolves
the missing-super finding. Remove the _loadFieldStars override at
animate/scenes/sol_trappist_cinematic.py#L240-L271, and add class attributes
consumed by the base implementation for the field-star radius and excluded
system_id.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f0c8d2ad-7011-4e4b-9ce8-a229954a8cdb
📒 Files selected for processing (6)
README.mdanimate/scenes/blender/README.mdanimate/scenes/sol_centauri_cinematic.pyanimate/scenes/sol_trappist_cinematic.pyrender.pytests/test_sol_trappist_cinematic.py
Gallery light/dark destination-chapter renders with Sol + TRAPPIST b–h billboards.
Skip unsafe super().__init__ (parent requires α Cen) with an explicit CodeQL annotation, and tighten orbit / field-star / inner-frame camera tests.
Extract SolScaleCinematicAnimator for Sol-open / billboard machinery; α Cen and TRAPPIST both inherit it and call super, clearing CodeQL missing-init.
Dive waypoints were Proxima leftovers that zoomed out to ~8000 AU after the 2 AU arrive hold, and the draw path hid the star above 80 AU — a black gap.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_sol_trappist_cinematic.py`:
- Around line 160-165: Update the dive-width assertions in the sampling loop
around animator._cameraState so they enforce strict non-increasing behavior
using a small absolute floating-point tolerance rather than multiplying holdHalf
and previous by 1.01. Preserve the existing comparisons and previous = halfWidth
tracking while allowing only negligible numerical error.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f4c4405b-7e97-479f-85c6-b4cfbd229088
⛔ Files ignored due to path filters (2)
output/animate/sol_trappist/blender/sol_trappist_cinematic_blender_dark.gifis excluded by!**/*.gifoutput/animate/sol_trappist/blender/sol_trappist_cinematic_blender_light.gifis excluded by!**/*.gif
📒 Files selected for processing (3)
animate/scenes/sol_centauri_cinematic.pyanimate/scenes/sol_trappist_cinematic.pytests/test_sol_trappist_cinematic.py
🚧 Files skipped from review as they are similar to previous changes (1)
- animate/scenes/sol_trappist_cinematic.py
Re-render after the dive black-gap fix so gallery assets match the tighten-only camera.
Use absolute float epsilon instead of 1% slack so the camera cannot quietly zoom out.
After the wide chain hold, draw a schematic HZ band, zoom to temperate candidates e/f, then ease out to the full b–h finale.
Filled annulus, dashed rims, screen-space label, and a steeper HZ camera so the band does not collapse into a foreshortened line.
Filled HZ annulus and screen-space label in the Sol→TRAPPIST blender gallery assets.
After the HZ band overview, shift the look-at point off the host onto the temperate candidates so the close-up reads as a planet beat, not a star frame.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@animate/scenes/sol_trappist_cinematic.py`:
- Around line 201-211: Update __init__ to validate that the catalog contains
TRAPPIST-1 b through TRAPPIST-1 h before rendering; collect all missing names
and raise ValueError listing them. Preserve _planetByName and _candidateFocusSol
behavior, and add a test covering a partial planet catalog.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d6ddef5-0b51-45dd-92bf-211cffd1c133
⛔ Files ignored due to path filters (2)
output/animate/sol_trappist/blender/sol_trappist_cinematic_blender_dark.gifis excluded by!**/*.gifoutput/animate/sol_trappist/blender/sol_trappist_cinematic_blender_light.gifis excluded by!**/*.gif
📒 Files selected for processing (3)
README.mdanimate/scenes/sol_trappist_cinematic.pytests/test_sol_trappist_cinematic.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_sol_trappist_cinematic.py
- README.md
Comment on lines
+201
to
+211
| def _planetByName(self, name: str) -> SystemPlanet: | ||
| for planet in self.trappistPlanets: | ||
| if planet.name == name: | ||
| return planet | ||
| raise KeyError(name) | ||
|
|
||
| def _candidateFocusSol(self, frame: int) -> np.ndarray: | ||
| """Look-at point between e and f so both HZ candidates stay in frame.""" | ||
| planetE = self._trappistPlanetPositionSol(self._planetByName('TRAPPIST-1 e'), frame) | ||
| planetF = self._trappistPlanetPositionSol(self._planetByName('TRAPPIST-1 f'), frame) | ||
| return 0.62 * planetE + 0.38 * planetF |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate the required planet rows before rendering.
_cameraState() calls _candidateFocusSol() for every frame. A StarSystem that passes the current constructor checks but lacks TRAPPIST-1 e or TRAPPIST-1 f fails later with KeyError, including during the Sol opening.
Validate the required TRAPPIST-1 b through TRAPPIST-1 h names in __init__. Raise ValueError with the missing names. Add a test for a partial planet catalog.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@animate/scenes/sol_trappist_cinematic.py` around lines 201 - 211, Update
__init__ to validate that the catalog contains TRAPPIST-1 b through TRAPPIST-1 h
before rendering; collect all missing names and raise ValueError listing them.
Preserve _planetByName and _candidateFocusSol behavior, and add a test covering
a partial planet catalog.
Queue Blender billboards as soon as the chain is readable, and never fall back to catalog scatter markers when a spin pack is present.
Re-render light/dark gallery assets after textured billboards lead the dive.
Shrink planet disks vs the host on first reveal, let e/f grow through a deeper HZ→candidate dive, and slow/extend those orbit holds.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_sol_trappist_cinematic.py`:
- Around line 224-231: Update the assertions in the test covering the HZ and
candidate holds to compare each hold duration against its corresponding zoom-in
duration, using the existing arrival and hold boundary constants; retain the
current minimum frame-count checks if they remain required.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ef6fc882-9696-431a-baa4-91094ccb1902
⛔ Files ignored due to path filters (2)
output/animate/sol_trappist/blender/sol_trappist_cinematic_blender_dark.gifis excluded by!**/*.gifoutput/animate/sol_trappist/blender/sol_trappist_cinematic_blender_light.gifis excluded by!**/*.gif
📒 Files selected for processing (3)
animate/scenes/sol_centauri_cinematic.pyanimate/scenes/sol_trappist_cinematic.pytests/test_sol_trappist_cinematic.py
🚧 Files skipped from review as they are similar to previous changes (2)
- animate/scenes/sol_centauri_cinematic.py
- animate/scenes/sol_trappist_cinematic.py
Accumulate motion days when slowing the finale, and stop culling e/f by semi-major axis once the camera pans onto them.
Light/dark gallery assets with continuous TRAPPIST motion and e/f visible through the candidate close-up.
Use the Earth blender half-width and an Earth-scale hero disk on e/f so the final zoom fills the frame like the Sol opening.
…se-up Gallery light/dark assets after matching the e/f beat to the Earth open frame.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_sol_trappist_cinematic.py`:
- Around line 319-332: Update the test around _drawOneTrappistPlanet so it does
not depend on the optional TRAPPIST-1 e spin pack: mock _queueBlenderBody to
capture the queued body and retain the existing scale assertion, or explicitly
skip the assertion when the pack is unavailable.
- Around line 274-281: Update the monotonicity assertion loop in the
motion-clock test around _trappistMotionDays to require each sampled current
value to be strictly greater than the previous value, preventing frozen frames
from passing.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ea0c3d2c-153d-43ea-a95a-70a280b24d4b
⛔ Files ignored due to path filters (2)
output/animate/sol_trappist/blender/sol_trappist_cinematic_blender_dark.gifis excluded by!**/*.gifoutput/animate/sol_trappist/blender/sol_trappist_cinematic_blender_light.gifis excluded by!**/*.gif
📒 Files selected for processing (3)
animate/scenes/sol_centauri_cinematic.pyanimate/scenes/sol_trappist_cinematic.pytests/test_sol_trappist_cinematic.py
🚧 Files skipped from review as they are similar to previous changes (2)
- animate/scenes/sol_centauri_cinematic.py
- animate/scenes/sol_trappist_cinematic.py
Comment on lines
+274
to
+281
| # Accumulated clock must keep advancing (no reverse) when the rate eases down. | ||
| days = [ | ||
| animator._trappistMotionDays(frame) | ||
| for frame in range(hzHold - 2, min(frames, candHold + 3)) | ||
| ] | ||
| self.assertGreater(days[-1], days[0]) | ||
| for previous, current in zip(days, days[1:], strict=False): | ||
| self.assertGreaterEqual(current, previous) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require every sampled motion-clock step to advance.
Line 281 permits an unchanged motion-clock value. This passes when the finale freezes on one or more frames. Use a strict comparison so the test enforces continuous orbital motion.
Proposed fix
- self.assertGreaterEqual(current, previous)
+ self.assertGreater(current, previous)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Accumulated clock must keep advancing (no reverse) when the rate eases down. | |
| days = [ | |
| animator._trappistMotionDays(frame) | |
| for frame in range(hzHold - 2, min(frames, candHold + 3)) | |
| ] | |
| self.assertGreater(days[-1], days[0]) | |
| for previous, current in zip(days, days[1:], strict=False): | |
| self.assertGreaterEqual(current, previous) | |
| # Accumulated clock must keep advancing (no reverse) when the rate eases down. | |
| days = [ | |
| animator._trappistMotionDays(frame) | |
| for frame in range(hzHold - 2, min(frames, candHold + 3)) | |
| ] | |
| self.assertGreater(days[-1], days[0]) | |
| for previous, current in zip(days, days[1:], strict=False): | |
| self.assertGreater(current, previous) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_sol_trappist_cinematic.py` around lines 274 - 281, Update the
monotonicity assertion loop in the motion-clock test around _trappistMotionDays
to require each sampled current value to be strictly greater than the previous
value, preventing frozen frames from passing.
Drop the Earth-sized dual-hero disks; zoom onto one HZ world at a time with disks that stay under orbital spacing, then pan to the next.
Gallery light/dark assets after one-planet-at-a-time HZ close-ups.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--system sol_trappistdestination chapter: Sol open → neighborhood cruise → TRAPPIST-1 at true Sol XYZ (~40.7 ly) → b–h finale--blender-bodiescomposites Sol packs + TRAPPIST-1 b–h spin billboards (host stays a scatter marker)Test plan
python -m unittest tests.test_sol_trappist_cinematicoutput/animate/sol_trappist/blender/Closes #71
Summary by CodeRabbit
New Features
Documentation
Tests