Non synchronous moon phases #36549
Merged
Non synchronous moon phases #36549
Conversation
This file contains 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
Qrox
reviewed
Dec 30, 2019
| static constexpr time_duration synodic_month = 29.530588853 * 1_days; | ||
| const time_duration moon_phase_duration = | ||
| calendar::season_from_default_ratio() * synodic_month; | ||
| // Switch moon phase at noon so it stays the same all night |
There was a problem hiding this comment.
This comment will not be necessarily true with non synchronous moon cycles.
There was a problem hiding this comment.
You're right that this was broken, but I think it was already broken before my changes. In any case, I've now fixed it, and added a test.
ZhilkinSerg
added
<Enhancement / Feature>
jbytheway
force-pushed
the
non_synchronous_moon_phases
branch
from
010156f to
77e7e5b
Compare
Previously the in-game lunar month was exactly one third of a season (30.333 days, by default). This meant that the lunar cycle matched perfectly with the seasons / years, unlike the real-world lunar cycle which is inconveniently mismatched with other calendar units. Switch it so that, for the default season length, the lunar cycle matches the real-world synodic month of about 29.5 days. Also tidy up the code so that it's clearer how this value is arrived at.
Pull these values out as global statics to make it clearer that the default season length is the real-world season length.
This was failing on Mingw, probably due to -ffast-math optimizations.
This was already theoretically implemented, but it didn't work. Fix it and add a test.
jbytheway
force-pushed
the
non_synchronous_moon_phases
branch
from
77e7e5b to
31bae0e
Compare
This one is in Catch2 internals and triggered by using generators.
This was referenced Jan 5, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
SUMMARY: Features "Make lunar cycle not exactly match season changes"
Purpose of change
Previously (since #35246) the in-game lunar month was exactly one third of a season (30.333 days, by default). This meant that the lunar cycle matched perfectly with the seasons / years, unlike the real-world lunar cycle which is inconveniently mismatched with other calendar units.
Describe the solution
Switch it so that, for the default season length, the lunar cycle matches the real-world synodic month of about 29.5 days.
Also tidy up the code so that it's clearer how this value is arrived at.
Add some tests for the lunar cycle code.
Describe alternatives you've considered
Having a fixed lunar month regardless of season length setting.
Testing
Unit tests, including the newly added test.
Additional context
I'm using Catch2 generators for this test. That's a relatively new feature which I believe we should be using in more places. This is a simple test case for that.