feat(SOLSYS-12): first Blender Earth flyby - #35
Merged
Conversation
Add body-centered flyby camera jobs, single-sun EEVEE rendering, GIF assembly, CLI --flyby, gallery links, and Earth flyby assets.
📝 WalkthroughWalkthroughThe PR implements the Blender planet flyby pipeline. It adds camera-path generation, themed Blender rendering, PNG-to-GIF assembly, CLI support, Earth assets, tests, documentation, and Blender backup-file exclusion. ChangesBlender flyby
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant BlenderCLI
participant renderPlanetFlyby
participant BlenderRenderer
participant GIFAssembler
BlenderCLI->>renderPlanetFlyby: pass flyby options
renderPlanetFlyby->>BlenderRenderer: submit themed job
BlenderRenderer-->>renderPlanetFlyby: write PNG frames
renderPlanetFlyby->>GIFAssembler: assemble PNG frames
GIFAssembler-->>BlenderCLI: return GIF paths
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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}
| if __name__ == '__main__': | ||
| exitCode = main() | ||
| try: | ||
| import bpy # type: ignore[import-not-found] # noqa: F401 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
animate/scenes/blender/render_flyby.py (2)
234-242: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the
bpycapability probe explicit.The import on Line 237 exists only to detect Blender. Static analysis reports it as unused.
importlib.util.find_spec('bpy')states the intent directly and avoids thenoqamarker.♻️ Proposed change
if __name__ == '__main__': exitCode = main() - try: - import bpy # type: ignore[import-not-found] # noqa: F401 - except ImportError: - raise SystemExit(exitCode) from None + import importlib.util + + if importlib.util.find_spec('bpy') is None: + raise SystemExit(exitCode) # Background mode exits on its own after the script; GUI should not SystemExit. if '--background' in sys.argv or '-b' in sys.argv: raise SystemExit(exitCode)🤖 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/blender/render_flyby.py` around lines 234 - 242, Replace the unused bpy import capability check in the __main__ block with an explicit importlib.util.find_spec('bpy') probe, removing the noqa suppression while preserving the existing SystemExit behavior for unavailable Blender and background mode.Source: Linters/SAST tools
84-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
'Specular Tint'in the candidate tuple is unreachable.The condition on Line 85 excludes
'Specular Tint', so the loop can never assign it. Remove the name from the tuple on Line 84 and drop the extra condition.♻️ Proposed change
- for inputName in ('Specular IOR Level', 'Specular', 'Specular Tint'): - if inputName in principled.inputs and inputName != 'Specular Tint': + for inputName in ('Specular IOR Level', 'Specular'): + if inputName in principled.inputs: principled.inputs[inputName].default_value = specular break🤖 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/blender/render_flyby.py` around lines 84 - 87, Update the candidate tuple in the material setup loop to remove 'Specular Tint', and remove the redundant inputName != 'Specular Tint' condition so the remaining valid specular inputs are handled directly.animate/scenes/blender/flyby_scene.py (3)
180-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the logging module over
renderPlanetFlybyis a library function thatrender.pyalso calls. The caller already prints its ownFlyby ready → …line for each returned path, so this produces duplicate output. Use a module logger, or remove this line and leave user-facing output to the CLI.🤖 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/blender/flyby_scene.py` at line 180, Remove the progress print from renderPlanetFlyby, since the CLI caller already reports each returned path; keep user-facing output centralized in render.py rather than adding duplicate logging.
98-110: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClose the frames even when the save fails, and simplify the duration math.
Lines 100-107 can raise. In that case, the loop at Lines 108-109 never runs and the opened images stay open. Wrap the save in
try/finally. Ruff also reports RUF046 on Line 99, becauseround()already returns aninthere.♻️ Proposed change
images = [Image.open(path).convert('RGB') for path in framePaths] - durationMs = max(int(round(1000 / fps)), 1) - images[0].save( - outputGif, - save_all=True, - append_images=images[1:], - duration=durationMs, - loop=0, - optimize=True, - ) - for image in images: - image.close() + durationMs = max(round(1000 / fps), 1) + try: + images[0].save( + outputGif, + save_all=True, + append_images=images[1:], + duration=durationMs, + loop=0, + optimize=True, + ) + finally: + for image in images: + image.close() return outputGif🤖 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/blender/flyby_scene.py` around lines 98 - 110, Update the frame-saving flow around images[0].save in the flyby scene to place the save operation inside a try/finally, ensuring every image in images is closed even when saving raises. Simplify durationMs by removing the redundant int() around round(), while preserving the existing minimum duration behavior.Source: Linters/SAST tools
20-20: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winResolve the render script path relative to this module.
RENDER_FLYBY_SCRIPTis a relative path._runBlenderFlybyJobpasses it to Blender at Line 125. The path only resolves when the process runs from the repository root.renderPlanetFlybyis a public package export, so importers can run from any directory.♻️ Proposed change
-RENDER_FLYBY_SCRIPT = Path('animate/scenes/blender/render_flyby.py') +RENDER_FLYBY_SCRIPT = Path(__file__).resolve().parent / 'render_flyby.py'🤖 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/blender/flyby_scene.py` at line 20, Update RENDER_FLYBY_SCRIPT to construct an absolute path from the module’s own location, targeting render_flyby.py relative to flyby_scene.py. Ensure _runBlenderFlybyJob passes this module-resolved path so the public renderPlanetFlyby entry point works regardless of the process working directory.
🤖 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/blender/flyby_camera.py`:
- Around line 41-49: Enforce the frameCount >= 2 contract in
buildFlybyCameraPath by raising ValueError before the sampling loop. In
animate/scenes/blender/flyby_scene.py lines 52-55, add the same validation
beside the theme check and replace max(frameCount, 2) with frameCount so invalid
values are rejected consistently.
In `@animate/scenes/blender/flyby_scene.py`:
- Around line 161-176: Update renderPlanetFlyby so buildFlybyJob uses a frames
directory under outputRoot, or rewrites the job’s outputDirectory to a
reproducible repository-relative path before writeFlybyJob persists it; do not
store a TemporaryDirectory path. Regenerate
output/animate/blender/earth_flyby_dark_job.json at line 668 and
output/animate/blender/earth_flyby_light_job.json at line 668 so each records
the corrected outputDirectory.
In `@animate/scenes/blender/render_flyby.py`:
- Around line 130-133: Enable nodes immediately after creating the material in
the material setup around lines 130-133 so _setPrincipled can apply the job
colorRgba. Also enable nodes immediately after creating the world in
animate/scenes/blender/render_flyby.py lines 90-112 so the theme strength
configuration is applied; both changes target the newly created datablocks
before their node trees are accessed.
In `@render.py`:
- Around line 188-193: Change the --frames argument in the argparse setup to
default to None instead of using 120 as a sentinel. Update the frame-resolution
logic around the branch at lines 316–326 to test for None and preserve an
explicitly supplied 120, while applying the appropriate orbit or flyby default
only when frames was omitted.
---
Nitpick comments:
In `@animate/scenes/blender/flyby_scene.py`:
- Line 180: Remove the progress print from renderPlanetFlyby, since the CLI
caller already reports each returned path; keep user-facing output centralized
in render.py rather than adding duplicate logging.
- Around line 98-110: Update the frame-saving flow around images[0].save in the
flyby scene to place the save operation inside a try/finally, ensuring every
image in images is closed even when saving raises. Simplify durationMs by
removing the redundant int() around round(), while preserving the existing
minimum duration behavior.
- Line 20: Update RENDER_FLYBY_SCRIPT to construct an absolute path from the
module’s own location, targeting render_flyby.py relative to flyby_scene.py.
Ensure _runBlenderFlybyJob passes this module-resolved path so the public
renderPlanetFlyby entry point works regardless of the process working directory.
In `@animate/scenes/blender/render_flyby.py`:
- Around line 234-242: Replace the unused bpy import capability check in the
__main__ block with an explicit importlib.util.find_spec('bpy') probe, removing
the noqa suppression while preserving the existing SystemExit behavior for
unavailable Blender and background mode.
- Around line 84-87: Update the candidate tuple in the material setup loop to
remove 'Specular Tint', and remove the redundant inputName != 'Specular Tint'
condition so the remaining valid specular inputs are handled directly.
🪄 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: 92bf146a-8c33-4d87-82d6-d0671daf01fc
⛔ Files ignored due to path filters (2)
output/animate/blender/earth_flyby_dark.gifis excluded by!**/*.gifoutput/animate/blender/earth_flyby_light.gifis excluded by!**/*.gif
📒 Files selected for processing (12)
.gitignoreREADME.mdanimate/scenes/blender/README.mdanimate/scenes/blender/__init__.pyanimate/scenes/blender/flyby_camera.pyanimate/scenes/blender/flyby_scene.pyanimate/scenes/blender/render_flyby.pyoutput/animate/blender/earth_body_scene.jsonoutput/animate/blender/earth_flyby_dark_job.jsonoutput/animate/blender/earth_flyby_light_job.jsonrender.pytests/test_blender_pipeline.py
Comment on lines
+41
to
+49
| def buildFlybyCameraPath( | ||
| displayRadiusAu: float, | ||
| frameCount: int = 72, | ||
| *, | ||
| bodySpinDeg: float = 140.0, | ||
| ) -> tuple[FlybyCameraSample, ...]: | ||
| samples: list[FlybyCameraSample] = [] | ||
| for frame in range(frameCount): | ||
| progress = frame / (frameCount - 1) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Missing frameCount >= 2 contract in the flyby path. Neither the camera-path builder nor the job builder rejects frameCount values below 2. buildFlybyCameraPath divides by frameCount - 1 before flybyCameraLocation can validate it, and buildFlybyJob clamps only the body-scene call. render.py blender --flyby --frames 1 reaches the division and raises ZeroDivisionError; --frames 0 yields an empty frames list.
animate/scenes/blender/flyby_camera.py#L41-L49: addif frameCount < 2: raise ValueError('frameCount must be >= 2')before the loop.animate/scenes/blender/flyby_scene.py#L52-L55: validateframeCount >= 2next to the theme check and replacemax(frameCount, 2)withframeCount.
📍 Affects 2 files
animate/scenes/blender/flyby_camera.py#L41-L49(this comment)animate/scenes/blender/flyby_scene.py#L52-L55
🤖 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/blender/flyby_camera.py` around lines 41 - 49, Enforce the
frameCount >= 2 contract in buildFlybyCameraPath by raising ValueError before
the sampling loop. In animate/scenes/blender/flyby_scene.py lines 52-55, add the
same validation beside the theme check and replace max(frameCount, 2) with
frameCount so invalid values are rejected consistently.
Comment on lines
+161
to
+176
| with tempfile.TemporaryDirectory(prefix=f'solsys_flyby_{stem}_{themeName}_') as temporary: | ||
| framesDirectory = Path(temporary) / 'frames' | ||
| job = buildFlybyJob( | ||
| planetName, | ||
| theme=themeName, | ||
| frameCount=frameCount, | ||
| resolution=resolution, | ||
| fps=fps, | ||
| framesDirectory=framesDirectory, | ||
| ) | ||
| jobPath = outputRoot / f'{stem}_flyby_{themeName}_job.json' | ||
| writeFlybyJob(job, jobPath) | ||
| _runBlenderFlybyJob(jobPath) | ||
| framePaths = sorted(framesDirectory.glob('frame_*.png')) | ||
| if not framePaths: | ||
| raise RuntimeError(f'No frames rendered for theme={themeName}') |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persisted flyby jobs record a temporary frames directory. renderPlanetFlyby places framesDirectory inside a TemporaryDirectory, stores that absolute path in the job under outputDirectory, and then writes the job to the durable output tree. The recorded path is deleted when the context manager exits, so the committed jobs cannot be replayed with the documented blender --background --python animate/scenes/blender/render_flyby.py -- <job.json> command. The paths also disclose the author's local macOS temporary directory layout.
animate/scenes/blender/flyby_scene.py#L161-L176: render frames into a directory underoutputRoot, or rewriteoutputDirectoryto a repository-relative path beforewriteFlybyJobpersists the job.output/animate/blender/earth_flyby_dark_job.json#L668-L668: regenerate this artifact after the fix sooutputDirectoryholds a reproducible path.output/animate/blender/earth_flyby_light_job.json#L668-L668: regenerate this artifact after the fix sooutputDirectoryholds a reproducible path.
📍 Affects 3 files
animate/scenes/blender/flyby_scene.py#L161-L176(this comment)output/animate/blender/earth_flyby_dark_job.json#L668-L668output/animate/blender/earth_flyby_light_job.json#L668-L668
🤖 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/blender/flyby_scene.py` around lines 161 - 176, Update
renderPlanetFlyby so buildFlybyJob uses a frames directory under outputRoot, or
rewrites the job’s outputDirectory to a reproducible repository-relative path
before writeFlybyJob persists it; do not store a TemporaryDirectory path.
Regenerate output/animate/blender/earth_flyby_dark_job.json at line 668 and
output/animate/blender/earth_flyby_light_job.json at line 668 so each records
the corrected outputDirectory.
Comment on lines
+130
to
+133
| material = bpy.data.materials.new(name=f'{name}FlybyMaterial') | ||
| roughness = 0.42 if theme == 'light' else 0.55 | ||
| specular = 0.35 if theme == 'light' else 0.22 | ||
| _setPrincipled(material, color=color, roughness=roughness, specular=specular) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Newly created datablocks may have no node tree. bpy.data.materials.new() and bpy.data.worlds.new() do not enable nodes in several Blender versions. Both call sites then read node_tree, find None, and skip the shading work. Set use_nodes = True on each datablock immediately after creation.
animate/scenes/blender/render_flyby.py#L130-L133: setmaterial.use_nodes = Trueafter thebpy.data.materials.new()call, otherwise_setPrincipledreturns early and the planet ignores the jobcolorRgba.animate/scenes/blender/render_flyby.py#L90-L112: setworld.use_nodes = Trueafter thebpy.data.worlds.new()call, otherwise the themestrengthvalue is never applied and the light and dark backgrounds differ only by flat color.
📍 Affects 1 file
animate/scenes/blender/render_flyby.py#L130-L133(this comment)animate/scenes/blender/render_flyby.py#L90-L112
🤖 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/blender/render_flyby.py` around lines 130 - 133, Enable nodes
immediately after creating the material in the material setup around lines
130-133 so _setPrincipled can apply the job colorRgba. Also enable nodes
immediately after creating the world in animate/scenes/blender/render_flyby.py
lines 90-112 so the theme strength configuration is applied; both changes target
the newly created datablocks before their node trees are accessed.
Comment on lines
188
to
193
| blenderParser.add_argument( | ||
| '--frames', | ||
| type=int, | ||
| default=120, | ||
| help='Number of orbit keyframes to sample (default: 120)', | ||
| help='Orbit keyframes for export, or flyby frames when --flyby (default: 120 / 72)', | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Replace the 120 sentinel with an explicit default of None.
Line 317 treats args.frames == 120 as "not specified". A user who explicitly runs --flyby --frames 120 receives 72 frames instead. Set the argparse default to None and resolve the value in each branch.
♻️ Proposed change
blenderParser.add_argument(
'--frames',
type=int,
- default=120,
+ default=None,
help='Orbit keyframes for export, or flyby frames when --flyby (default: 120 / 72)',
) if args.command == 'blender':
if args.flyby:
- flybyFrames = 72 if args.frames == 120 else args.frames
+ flybyFrames = 72 if args.frames is None else args.frames
gifPaths = renderPlanetFlyby(
args.body,
theme=args.theme,
frameCount=flybyFrames,
outputDirectory=args.output_dir,
)
for gifPath in gifPaths:
print(f'Flyby ready → {gifPath}')
return
scenePath = exportPlanetBodyScene(
args.body,
- frameCount=args.frames,
+ frameCount=120 if args.frames is None else args.frames,
outputDirectory=args.output_dir,
)Also applies to: 316-326
🤖 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 `@render.py` around lines 188 - 193, Change the --frames argument in the
argparse setup to default to None instead of using 120 as a sentinel. Update the
frame-resolution logic around the branch at lines 316–326 to test for None and
preserve an explicitly supplied 120, while applying the appropriate orbit or
flyby default only when frames was omitted.
4 tasks
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
render.py blender --body Earth --flybyCloses #12
Test plan
python -m unittest discover -s tests -vrender.py blender --body Earth --flyby --theme all --frames 72earth_flyby_{light,dark}.gifin the PRSummary by CodeRabbit
New Features
--flybyand--themeoptions to the Blender command.Documentation
Tests
Chores