Skip to content

AstroCalc: Fix RTS#3028

Merged
alex-w merged 8 commits into
Stellarium:masterfrom
worachate001:rts-fix
Feb 3, 2023
Merged

AstroCalc: Fix RTS#3028
alex-w merged 8 commits into
Stellarium:masterfrom
worachate001:rts-fix

Conversation

@worachate001
Copy link
Copy Markdown
Member

Description

The problems are:

This PR will fix them by:

  1. When select the Moon (or other objects). Info will show '(previous day)' or '(next day)' after the time to inform us that the calculated event doesn't happen on current day.
  2. RTS table in AstroCalc will show the events that occur on the same date in the same row. This should make it easier to read/process in other applications.
  3. Fixing above helps us eliminate time spikes issue.

Moon-RTS

Fixes #2990 and remaining issue in #1482

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • This change requires a documentation update

How Has This Been Tested?

Test Configuration:

  • Operating system: Manjaro 22.0.1
  • Graphics Card: -

Checklist:

  • My code follows the code style of this project.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (header file)
  • I have updated the respective chapter in the Stellarium User Guide
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@worachate001 worachate001 added this to the 23.1 milestone Jan 28, 2023
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jan 28, 2023

Great PR! Please pay attention to the following items before merging:

Files matching src/**/*.cpp:

  • Are possibly unused includes removed?

This is an automatically generated QA checklist based on modified files.

@alex-w
Copy link
Copy Markdown
Member

alex-w commented Jan 28, 2023

Thank you very much for the fix!

I see sentences “previous day/next day” - why not “yesterday/tomorrow”?

@10110111
Copy link
Copy Markdown
Contributor

I see sentences “previous day/next day” - why not “yesterday/tomorrow”?

It makes sense if current day is not the realtime "today", and not even a simulated "today".

@worachate001
Copy link
Copy Markdown
Member Author

Thank you very much for the fix!

I see sentences “previous day/next day” - why not “yesterday/tomorrow”?

This actually came across my mind at one point during development. But I though the same with @10110111 that it's mostly simulated time. Using yesterday/tomorrow may not make sense.

@alex-w
Copy link
Copy Markdown
Member

alex-w commented Jan 28, 2023

This actually came across my mind at one point during development. But I though the same with @10110111 that it's mostly simulated time. Using yesterday/tomorrow may not make sense.

OK, I got it

@gzotti
Copy link
Copy Markdown
Member

gzotti commented Jan 28, 2023

Many thanks for this. StelObject::getInfoMap() may be fixed with this update too. The transit, rise and set entries are QStrings which may have (previous day) or (next day) appended (I think we usually don't translate the InfoMap to make it easier for programs which are using it). I am not sure how to deal with the {transit|rise|set}-dhr entries, those are numerical. Allow hours >24 and <0? Or just keep as-is and let the user of the data find out by parsing the other string?

@worachate001
Copy link
Copy Markdown
Member Author

Many thanks for this. StelObject::getInfoMap() may be fixed with this update too. The transit, rise and set entries are QStrings which may have (previous day) or (next day) appended (I think we usually don't translate the InfoMap to make it easier for programs which are using it). I am not sure how to deal with the {transit|rise|set}-dhr entries, those are numerical. Allow hours >24 and <0? Or just keep as-is and let the user of the data find out by parsing the other string?

I'm thinking about another way that might be better. Create getRTSTimeOfDate() that returns RTS of current date (doing it inside getRTSTime() may be too complicated).

@gzotti
Copy link
Copy Markdown
Member

gzotti commented Jan 29, 2023

OK. Just that the InfoMap always needs updates in line with the InfoString.

@worachate001 worachate001 marked this pull request as draft January 30, 2023 09:42
@worachate001
Copy link
Copy Markdown
Member Author

worachate001 commented Jan 31, 2023

I'm ditching the idea of displaying 'previous day' or 'next day'. It was proposed earlier because that is an easy way with minimal tweak in existing codes. But it becomes more complicated for StelObject::getInfoMap().

New update will show RTS that occur on current date only (RTS will change at midnight, not seem to change in unpredictable way during a day like before). It also makes more sense and in line with the way they show RTS in astronomical almanacs. Please check StelObject::getInfoMap() to see if it's okay with new changes because I'm not familiar with its structure.

Update: Made it draft because there is a potential bug for non-solar system objects.

@worachate001 worachate001 marked this pull request as ready for review January 31, 2023 09:38
@github-actions github-actions Bot requested a review from alex-w January 31, 2023 09:38
@worachate001 worachate001 marked this pull request as draft January 31, 2023 10:10
@alex-w
Copy link
Copy Markdown
Member

alex-w commented Jan 31, 2023

Hmm... I see serious regress of performance on the Mac when the Moon is selected

Comment thread src/core/modules/Planet.cpp
Comment thread src/core/modules/Planet.cpp
@worachate001 worachate001 marked this pull request as ready for review February 1, 2023 10:24
@github-actions github-actions Bot requested a review from alex-w February 1, 2023 10:24
@worachate001
Copy link
Copy Markdown
Member Author

Thanks @alex-w. I have no performance hit so I'm not sure it's getting better or not. The new update should work now for all objects.

@alex-w alex-w merged commit 3364be7 into Stellarium:master Feb 3, 2023
@worachate001 worachate001 deleted the rts-fix branch February 3, 2023 23:22
@github-actions
Copy link
Copy Markdown

Hello @worachate001!

The enhancement or feature has been merged into source code and you may test it via building Stellarium from source code or wait the weekly development snapshot...

@github-actions
Copy link
Copy Markdown

Hello @worachate001!

The fix has been merged into source code and you may test it via building Stellarium from source code or wait the weekly development snapshot...

@alex-w alex-w added state: published The fix has been published for testing in weekly binary package and removed state: fixed labels Mar 13, 2023
@github-actions
Copy link
Copy Markdown

Hello @worachate001!

Please check the fresh version (development snapshot) of Stellarium:
https://github.com/Stellarium/stellarium-data/releases/tag/weekly-snapshot

@alex-w alex-w removed the state: published The fix has been published for testing in weekly binary package label Mar 27, 2023
@github-actions
Copy link
Copy Markdown

Hello @worachate001!

Please check the latest stable version of Stellarium:
https://github.com/Stellarium/stellarium/releases/latest

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.

moon culmination data problem

4 participants