Add landscape composite horizon directions and annotation toggle#22
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR improves the Photography Guide “Landscape composite” modal by making the composite observer-aware (sun azimuth/altitude), adding horizon/compass direction annotations, and introducing a single toggle to show/hide composite annotations.
Changes:
- Extend landscape composite placements with
sunAltitudeDeg,sunAzimuthDeg, andisAboveHorizon, and use them to suppress below-horizon rendering. - Add 16-point compass direction ticks/labels aligned to the MAX sun azimuth, plus a modal toggle to show/hide composite annotations.
- Add a unit test covering observer-aware placement metadata and horizon-flagging; bump mobile version and update changelog.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/mobile/src/utils/photographyGuide.ts | Adds observer-aware sun azimuth/altitude metadata and an isAboveHorizon flag to composite placements (plus fallback computation). |
| apps/mobile/src/screens/PhotographyGuideScreen.tsx | Renders horizon compass markers and adds a UI toggle to show/hide composite annotations; suppresses rendering for below-horizon placements. |
| apps/mobile/tests/photography-guide.test.ts | Adds a regression test asserting observer-aware layouts expose azimuth/altitude and below-horizon flags. |
| apps/mobile/package.json | Bumps @eclipse-timer/mobile version to 1.1.38. |
| CHANGELOG.md | Documents the new composite markings/toggle and below-horizon suppression behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <View | ||
| style={[ | ||
| styles.compositeDirectionTick, | ||
| { top: activeCompositeHorizonY - 6 }, |
There was a problem hiding this comment.
The horizon compass marker wrapper is offset by left: marker.x - 10, but the tick has no horizontal offset within that wrapper, so the tick renders ~10px left of the intended azimuth line while the label is centered. Consider centering the tick on marker.x (e.g., by offsetting the tick inside the wrapper or changing the wrapper positioning strategy) so tick and label align.
| { top: activeCompositeHorizonY - 6 }, | |
| { top: activeCompositeHorizonY - 6, left: 10 }, |
There was a problem hiding this comment.
Summary
- Fixed the horizon compass tick alignment in the landscape composite overlay by adding a horizontal offset (
left: 10) to the tick style, so each tick now centers on the same azimuth line as its corresponding label. apps/mobile/src/screens/PhotographyGuideScreen.tsxL456-L468 - Committed the change on the current branch with message:
Fix horizon marker tick centering in composite overlay.
Testing
- ✅
pnpm -C apps/mobile typecheck ⚠️ Screenshot not captured: this is a React Native mobile screen change and there is no runnable browser target in this environment to renderPhotographyGuideScreenvia the browser tool.
| const LANDSCAPE_HORIZONTAL_FOV_DEG_24MM = 74; | ||
| const COMPASS_MARKERS = [ | ||
| { label: "N", azimuthDeg: 0 }, |
There was a problem hiding this comment.
LANDSCAPE_HORIZONTAL_FOV_DEG_24MM is defined here and also in apps/mobile/src/utils/photographyGuide.ts (same value). Duplicating this constant risks the UI compass markers drifting out of sync with the layout math if one side changes. Consider exporting the FOV constant from the utils module (or a shared constants module) and reusing it here.
There was a problem hiding this comment.
Summary
- Addressed reviewer feedback by removing duplicated composite math definitions in the screen and reusing shared exports from
photographyGuide.ts(LANDSCAPE_HORIZONTAL_FOV_DEG_24MMandnormalizeSignedDeltaDeg). apps/mobile/src/screens/PhotographyGuideScreen.tsxL17-L24 apps/mobile/src/utils/photographyGuide.tsL51-L53 apps/mobile/src/utils/photographyGuide.tsL119-L122 - Fixed horizon marker rendering alignment by centering the tick on the same x-position as its label (
left: 10inside the wrapper). apps/mobile/src/screens/PhotographyGuideScreen.tsxL453-L471 - Updated composite rendering behavior so below-horizon filtering applies to sun/moon/corona visuals only, while shot index tags and clamp indicators are still controlled by
showCompositeMarkings. apps/mobile/src/screens/PhotographyGuideScreen.tsxL484-L578 - Updated toggle copy to accurately reflect that clamp indicators are also toggled. apps/mobile/src/screens/PhotographyGuideScreen.tsxL409-L412
- Improved test clarity/coverage by renaming the below-horizon test and adding a daylight scenario that asserts at least one placement is above the horizon. apps/mobile/tests/photography-guide.test.tsL271-L328
- Committed on the current branch (
aa270c4) and created a follow-up PR viamake_pr.
Testing
- ✅
pnpm -C apps/mobile test tests/photography-guide.test.ts - ✅
pnpm -C apps/mobile typecheck ⚠️ N/A (React Native screen change; browser screenshot capture is not applicable in this environment)
| function normalizeSignedDeltaDeg(fromDeg: number, toDeg: number) { | ||
| const delta = ((toDeg - fromDeg + 540) % 360) - 180; | ||
| return delta === -180 ? 180 : delta; | ||
| } |
There was a problem hiding this comment.
normalizeSignedDeltaDeg duplicates the same function in apps/mobile/src/utils/photographyGuide.ts. To avoid subtle divergence in angle wrap behavior between layout computation and compass marker placement, consider exporting and reusing a single implementation.
There was a problem hiding this comment.
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.
c0d3rb4b4
deleted the
codex/add-sun-visibility-toggle-and-direction-markings
branch
Motivation
Description
buildLandscapeCompositeLayout:sunAltitudeDeg,sunAzimuthDeg, andisAboveHorizon, and use these to suppress sun/moon rendering when below horizon (file:apps/mobile/src/utils/photographyGuide.ts).apps/mobile/src/screens/PhotographyGuideScreen.tsx).showCompositeMarkingstoggle button in the landscape composite modal that shows/hides horizon ticks, compass labels, shot index tags, and clamp indicators (file:apps/mobile/src/screens/PhotographyGuideScreen.tsx).isAboveHorizonis false and to conditionally render shot numbers/clamp indicators based on the toggle; add supporting styles.apps/mobile/tests/photography-guide.test.ts).Testing
pnpm -C apps/mobile test tests/photography-guide.test.ts, and all tests passed (including the new regression): success.pnpm -C apps/mobile typecheck, and typechecking completed with no errors: success.Codex Task