Skip to content

Add equator and ecliptic overlays (#109)#111

Open
alexchandel wants to merge 2 commits into
da-luce:mainfrom
alexchandel:feat-eq
Open

Add equator and ecliptic overlays (#109)#111
alexchandel wants to merge 2 commits into
da-luce:mainfrom
alexchandel:feat-eq

Conversation

@alexchandel
Copy link
Copy Markdown

@alexchandel alexchandel commented Apr 17, 2026

Summary

  • add --equator and --ecliptic options for the polar projection
  • render the celestial equator and ecliptic overlays
  • draw the ecliptic in yellow when colors are enabled
  • add test coverage for the new coordinate/rendering path
  • fixes Ecliptic or polar equator? #109

@alexchandel alexchandel changed the title Add equator and ecliptic overlays Add equator and ecliptic overlays (#109) Apr 17, 2026
@alexchandel
Copy link
Copy Markdown
Author

@da-luce can I get a workflow approval?

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 17, 2026

Codecov Report

❌ Patch coverage is 94.19087% with 14 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/drawing.c 87.38% 3 Missing and 11 partials ⚠️
Files with missing lines Coverage Δ
src/coord.c 64.91% <100.00%> (+5.72%) ⬆️
test/coord_test.c 100.00% <100.00%> (ø)
test/drawing_test.c 89.24% <100.00%> (+7.73%) ⬆️
src/drawing.c 56.17% <87.38%> (+14.13%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@da-luce
Copy link
Copy Markdown
Owner

da-luce commented Apr 17, 2026

Just checked this out--awesome! I'll try and take a closer look soon to better understand the code and then merge this in

@alexchandel
Copy link
Copy Markdown
Author

Thx! I can add some more test coverage if you need. draw_polyline_connected() is very general-purpose / arguably overkill, but was the only way I could get the corners consistently correct and I thought "why not make it general?"

@da-luce
Copy link
Copy Markdown
Owner

da-luce commented Apr 22, 2026

Thx! I can add some more test coverage if you need. draw_polyline_connected() is very general-purpose / arguably overkill, but was the only way I could get the corners consistently correct and I thought "why not make it general?"

This is exactly how I've tried approach drawing.c, so it fits right in! Would you be able to add a few more coverage tests? I know the drawing ones are annoying to make but I plan on using draw_polyline_connected() to draw a full equatorial grid sometime soon (🤞), so it'd be good for that function to have good coverage

@alexchandel
Copy link
Copy Markdown
Author

Expanded the tests, think I got everything worth covering. Can you rerun the workflow? @codecov will you do that yourself, bot?

There might be some inspiration in render_reference_circle() to be taken for gridlines, it's somewhat general in sampling+projecting+drawing a celestial reference circle.

@alexchandel
Copy link
Copy Markdown
Author

@da-luce needs workflow approval again apparently

@alexchandel
Copy link
Copy Markdown
Author

There is also a bikeshedding question of -e / -E for --equator / --ecliptic. I figured "equator" was slightly more common so it got -e, but I could see it either way.

@da-luce
Copy link
Copy Markdown
Owner

da-luce commented May 2, 2026

I honestly don't have a huge preference, especially because I'll reorganize the API in 2.0. Stuff has been busy and I haven't had time to really look through this but I'll get it merged in the next few weeks

@alexchandel
Copy link
Copy Markdown
Author

No rush. There are a couple corner case "partials" in the codecov still too, lmk if you need those to be covered. I've been using this build locally for a while, haven't seen any issues.

I've noticed that if it's left running for like a full day, astroterm appears behind when I check in on it. Not sure if that's because of OS sleeping, or drift in the "game loop" logic, but worth noting.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ecliptic or polar equator?

2 participants