Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring the support of the on-screen stars names #2005

Merged
merged 15 commits into from
Nov 11, 2021

Conversation

alex-w
Copy link
Member

@alex-w alex-w commented Nov 2, 2021

This pull request is introduced the additional config options to show extra designations as on-screen names of stars.

Fixes #1951 (issue)

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

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
  • 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

@github-actions github-actions bot requested a review from gzotti November 2, 2021 13:02
@alex-w alex-w added this to the 0.21.3 milestone Nov 2, 2021
@github-actions
Copy link

github-actions bot commented Nov 2, 2021

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

Files matching src/**/*.cpp:

  • Are possibly unused includes removed?

Files matching guide/**:

  • Did you remember to update screenshots to match new updates?
  • Did you remember to grammar check in changed part of documentation?

This is an automatically generated QA checklist based on modified files

@alex-w alex-w marked this pull request as draft November 2, 2021 13:03
@alex-w alex-w added this to To do in GUI via automation Nov 2, 2021
@alex-w alex-w added the enhancement Improve existing functionality label Nov 2, 2021
@github-actions
Copy link

github-actions bot commented Nov 2, 2021

Hello @alex-w! Thank you for this enhancement.

@alex-w alex-w marked this pull request as ready for review November 2, 2021 14:18
Copy link
Member

@gzotti gzotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle a nice idea. However, some things should be fixed before merge:

"old" double star designations: in contrast to "new" ones? Or in contrast to showing "no labels"?
It seems it currently is "old" or "none". If I check table C.9 in SUG C.4.2, I can see "old" (obsolete) ID and "new" (modern) ID listed, the difference is the ignorance of most modern scientists and/or computer/printing systems to use Greek (or even just lowercase!) letters. The numbers within the catalogs are still in wide use, just as the Greek letters for the catalogs have been used in typeset editions for many decades. These old catalog spellings are still widely known and may be easier for the human reader. So, I can imagine it is useful to show either "old" or "new" or "no" double star labels. In addition, table C.9 needs additions: OCC, BAS, KOH? (just look at TAU head) Others?

Also, double star wins over var star label? Again, look at TAU head. SZ Tau is labelled when I select Var.stars, but only OCC926 when I select both Dbl. stars and Var.stars at the same time. And it has no label if "Designations" is disabled. IMHO if both Dbl.&Var stars are selected, both labels should be shown, at danger of clutter. This would also hit the appearance of Bayer/Flamsted labels which may have to have double or var (or both) designations added.

The next level would be a selection of which double star catalogs the user might be interested. But this may be another topic.

@alex-w
Copy link
Member Author

alex-w commented Nov 7, 2021

And it has no label if "Designations" is disabled. IMHO if both Dbl.&Var stars are selected, both labels should be shown, at danger of clutter.

Why? One star may have 3 designation from catalogs of doubles, one designation as variable star, plus Bayer and Flamesteed designation and one proper name, so, from your logic we should show all of them on the screen now?

Or you want to see better description for checkboxes and in the SUG?

@alex-w
Copy link
Member Author

alex-w commented Nov 7, 2021

"old" double star designations: in contrast to "new" ones? Or in contrast to showing "no labels"?

The modern designation of double stars start with WDS prefix :)

@gzotti
Copy link
Member

gzotti commented Nov 7, 2021

Why? One star may have 3 designation from catalogs of doubles, one designation as variable star, plus Bayer and Flamesteed designation and one proper name, so, from your logic we should show all of them on the screen now?

In fact, yes. If I check both double and var labels, and a star has both labels, still only one is shown. Why does the double star label "win" in this case? So, in this case we would need both labels. On Bayer/Flamsted I am less sure. Most (if not all) Bayer stars have Flamsted numbers, but tradition prefers Bayer over Flamsted where available. But if there are Bayer/Flamsted and the star has additional double or var star labels (or both? I don't have an example now), it can be reasoned to add those labels as well.

The "use designations..." checkbox switches off the proper name as screen label, in favour of the
( ((Bayer|Flamsted)[+Dbl][+Var]) | HIP). That's OK.

Or you want to see better description for checkboxes and in the SUG?

I don't understand the "old" part of the double star checkbox labels. These catalogs and names are still in use. The editors or typesetters of the latest edition of WDS seem to be unwilling or unable to print lowercase and Greek letters (?) so they declared the classical abbreviations obsolete, replacing them by 1-4 capital ASCII letters only. (OK, plus numbers and space for "H [1-6]") There is no informational difference between OΣ22 and STT22, both can be identified from SUG Table C.9. So, being able to select between "old" or "new" WDS designations as labels for double stars would be nice. And we need additions in the SUG what OCC, BAS, KOH (and maybe others) mean.

@gzotti
Copy link
Member

gzotti commented Nov 7, 2021

WDS numbers may be more systematic, but are certainly less human readable. You could either say "old" (Struve etc) vs, "new" WDS numbers (then we should label double stars with the WDS strings), or old and new catalog label spelling (as I understood previously). If you select "old" vs "WDS" labels, I'd wish to see the older spelling ("obsolete ID" in SUG Table C.9) for the "old" labels.

@alex-w
Copy link
Member Author

alex-w commented Nov 7, 2021

In fact, yes.

No, no and non againg. Please do not mix on-screen labels and list of designation

If I check both double and var labels, and a star has both labels, still only one is shown.

Yes, this is correct behaviour. All objects in Stellarium has once label on the screen. From your logic you want to see on the screen something like: Altair (Atair) - α Aql - 53 Aql - GMC 5 - SMR 5 - DAL 27 - Σ II 10 - HIP 97649. This is terrible!

Why does the double star label "win" in this case?

By historical reasons - most double stars catalogs and their designations was created before catalog of variable stars was born.

So, in this case we would need both labels.

On the screen we should see only one label, which has high priority.

On Bayer/Flamsted I am less sure. Most (if not all) Bayer stars have Flamsted numbers, but tradition prefers Bayer over Flamsted where available. But if there are Bayer/Flamsted and the star has additional double or var star labels (or both? I don't have an example now), it can be reasoned to add those labels as well.

See my example with Altair - you want to get an unreadable sky.

The "use designations..." checkbox switches off the proper name as screen label, in favour of the
( ((Bayer|Flamsted)[+Dbl][+Var]) | HIP). That's OK.

That's not OK, because the full logic to show the designation is: Bayer|Flamsted|Dbl|Var|HIP

I don't understand the "old" part of the double star checkbox labels.

Just replace the "old" to "traditional" word

These catalogs and names are still in use. The editors or typesetters of the latest edition of WDS seem to be unwilling or unable to print lowercase and Greek letters (?) so they declared the classical abbreviations obsolete, replacing them by 1-4 capital ASCII letters only. (OK, plus numbers and space for "H [1-6]")

SIMBAD uses latinized designations for double stars also.

There is no informational difference between OΣ22 and STT22, both can be identified from SUG Table C.9. So, being able to select between "old" or "new" WDS designations as labels for double stars would be nice.

OΣ22 and STT22 is traditinal designations for double stars from Otto von Struve catalog. In the modern catalogs I saw both versions of designations. The really modern double stars has official WDS designation only.

And we need additions in the SUG what OCC, BAS, KOH (and maybe others) mean.

OK

@gzotti
Copy link
Member

gzotti commented Nov 7, 2021

If I check both double and var labels, and a star has both labels, still only one is shown.

Yes, this is correct behaviour. All objects in Stellarium has once label on the screen. From your logic you want to see on the screen something like: Altair (Atair) - α Aql - 53 Aql - GMC 5 - SMR 5 - DAL 27 - Σ II 10 - HIP 97649. This is terrible!

No, I never said this. The max would be "Bayer-Vxxx-STTyyy", which would likely still be shorter than a single WDS label.

Why does the double star label "win" in this case?

By historical reasons - most double stars catalogs and their designations was created before catalog of variable stars was born.

This needs to be stated in the SUG then.

So, in this case we would need both labels.

On the screen we should see only one label, which has high priority.

What if a user is more interested in var stars?

On Bayer/Flamsted I am less sure. Most (if not all) Bayer stars have Flamsted numbers, but tradition prefers Bayer over Flamsted where available. But if there are Bayer/Flamsted and the star has additional double or var star labels (or both? I don't have an example now), it can be reasoned to add those labels as well.

See my example with Altair - you want to get an unreadable sky.

Definitely not. Just show what the user has asked to show with the checkboxes.

The "use designations..." checkbox switches off the proper name as screen label, in favour of the
( ((Bayer|Flamsted)[+Dbl][+Var]) | HIP). That's OK.

That's not OK, because the full logic to show the designation is: Bayer|Flamsted|Dbl|Var|HIP

OK. If you insist on just one designation, clearly say so in the SUG. Currently I can read a footnote saying "All of these items work independently from the parent option."

I don't understand the "old" part of the double star checkbox labels.

Just replace the "old" to "traditional" word

Yes.

OΣ22 and STT22 is traditinal designations for double stars from Otto von Struve catalog. In the modern catalogs I saw both versions of designations. The really modern double stars has official WDS designation only.

Currently I can select "old" (-->traditional") label or "no label". Would it be useful to show "WDS label" as alternative?

@alex-w
Copy link
Member Author

alex-w commented Nov 7, 2021

Yes, this is correct behaviour. All objects in Stellarium has once label on the screen. From your logic you want to see on the screen something like: Altair (Atair) - α Aql - 53 Aql - GMC 5 - SMR 5 - DAL 27 - Σ II 10 - HIP 97649. This is terrible!

No, I never said this. The max would be "Bayer-Vxxx-STTyyy", which would likely still be shorter than a single WDS label.

Yes, you said this, because GMC 5 - SMR 5 - DAL 27 - Σ II 10 is traditional designations of double stars for Altair and from your logic and latest remark we should show α Aql - GMC 5 - SMR 5 - DAL 27 - Σ II 10. Do you see problem here? Stellarium always was show an one designation on the screen for any object - why you want to change this behaviour? How this change is related to the requirement of reduce the number of labels on the screen?

By historical reasons - most double stars catalogs and their designations was created before catalog of variable stars was born.

This needs to be stated in the SUG then.

But this is obviously reason.

On the screen we should see only one label, which has high priority.

What if a user is more interested in var stars?

Who do not allow user to disable previous checkbox? What if a user is more interested in SAO designations?

See my example with Altair - you want to get an unreadable sky.

Definitely not. Just show what the user has asked to show with the checkboxes.

See changes in the code.

That's not OK, because the full logic to show the designation is: Bayer|Flamsted|Dbl|Var|HIP

OK. If you insist on just one designation, clearly say so in the SUG.

This is standard behaviour in Stellarium (and probably in all planetariums). I understand your reasons but, in re-phrase, writing "open the box to eating pizza" on pizza box is a bit strange IMHO?

Currently I can read a footnote saying "All of these items work independently from the parent option."

Yes, because enabling/disabling HIP designations is not touches visibility of common names or Bayer designations. Probably this is my bad English, but OK, I'll add an additional notes in the SUG.

Currently I can select "old" (-->traditional") label or "no label". Would it be useful to show "WDS label" as alternative?

Why? Traditional labels was requested many times, but no WDS labels.

@gzotti
Copy link
Member

gzotti commented Nov 7, 2021

Yes, this is correct behaviour. All objects in Stellarium has once label on the screen. From your logic you want to see on the screen something like: Altair (Atair) - α Aql - 53 Aql - GMC 5 - SMR 5 - DAL 27 - Σ II 10 - HIP 97649. This is terrible!

No, I never said this. The max would be "Bayer-Vxxx-STTyyy", which would likely still be shorter than a single WDS label.

Yes, you said this, because GMC 5 - SMR 5 - DAL 27 - Σ II 10 is traditional designations of double stars for Altair and from your logic and latest remark we should show α Aql - GMC 5 - SMR 5 - DAL 27 - Σ II 10. Do you see problem here? Stellarium always was show an one designation on the screen for any object - why you want to change this behaviour? How this change is related to the requirement of reduce the number of labels on the screen?

There is certainly a common preference also in the zoo of popular double star catalogs. Just as Bayer wins over Flamsted, one double star catalog is more popular than the next, so there is no need to have GMC (what is this?), SMR (what is this?), DAL (what is this?) plus a Struve number as screen label. AFAICS the three catalogs are not even given by SIMBAD and they are not listed in the SUG! (Please add these catalog names where they fit!)

We could say, no extra label if Bayer/Flamsted exists. But if an otherwise unlabelled star is both double and variable, and the user selected to show both, the label should reflect that.

By historical reasons - most double stars catalogs and their designations was created before catalog of variable stars was born.

This needs to be stated in the SUG then.

But this is obviously reason.

Not really.

On the screen we should see only one label, which has high priority.

What if a user is more interested in var stars?

Who do not allow user to disable previous checkbox? What if a user is more interested in SAO designations?

SAO (like HR, HD, GSC, FK5, ...) is not a current option.

OK. If you insist on just one designation, clearly say so in the SUG.

This is standard behaviour in Stellarium (and probably in all planetariums). I understand your reasons but, in re-phrase, writing "open the box to eating pizza" on pizza box is a bit strange IMHO?

Currently I can read a footnote saying "All of these items work independently from the parent option."

Yes, because enabling/disabling HIP designations is not touches visibility of common names or Bayer designations. Probably this is my bad English, but OK, I'll add an additional notes in the SUG.

Then the extra options depend on the parent! And var depends on being non-double, etc. The dependence rules need to be given.

Currently I can select "old" (-->traditional") label or "no label". Would it be useful to show "WDS label" as alternative?

Why? Traditional labels was requested many times, but no WDS labels.

Then simply call the flag "double star labels" not "old double star labels". Just that "old" implies a "new" alternative. Maybe add "traditional" in the tooltip.

@alex-w
Copy link
Member Author

alex-w commented Nov 7, 2021

We could say, no extra label if Bayer/Flamsted exists. But if an otherwise unlabelled star is both double and variable, and the user selected to show both, the label should reflect that.

I disagree, because we should always show both Bayer and Flamesteed designations if their existing to complete follow your proposal.

@alex-w
Copy link
Member Author

alex-w commented Nov 7, 2021

Then the extra options depend on the parent! And var depends on being non-double, etc. The dependence rules need to be given.

How it possible? The parent option enabled/disabled show common names for stars. Where you see the dependence for double star designation or HIP number?

@alex-w alex-w requested a review from gzotti November 7, 2021 18:19
@gzotti
Copy link
Member

gzotti commented Nov 7, 2021

Uh. I just was about to commit the updated documentation and fixed tooltip strings... If you want your way, you can revert your last commit. Else I will have to cleanup a conflict. The major point of misunderstanding was that your implementation (sequence with preference to double stars where available) did not fit your documentation ("append").

@alex-w
Copy link
Member Author

alex-w commented Nov 7, 2021

I think this is ready to merge now...

@alex-w alex-w moved this from To do to In progress in GUI Nov 7, 2021
@alex-w
Copy link
Member Author

alex-w commented Nov 7, 2021

@gzotti should we sort the list of designations in the SUG (C.6) by asc. in second column?

@gzotti
Copy link
Member

gzotti commented Nov 7, 2021

Yes, alphabetic sorting is a good idea. Maybe even make a horizontal line between classic catalogs and your new WDS observer tags. I have no knowledge which are the "catalogs" (single publications) or "observer entries in WDS lists" (like those tagged OCC).

@alex-w
Copy link
Member Author

alex-w commented Nov 7, 2021

Yes, alphabetic sorting is a good idea.

OK

Maybe even make a horizontal line between classic catalogs and your new WDS observer tags. I have no knowledge which are the "catalogs" (single publications) or "observer entries in WDS lists" (like those tagged OCC).

This is really problematic, because collection of few lists is have no difference with catalog (one big list).

Maybe just commenting the fact about famously designations and couple photos from books (from XIX-XX century’s)?

guide/app_star_catalogue.tex Outdated Show resolved Hide resolved
guide/app_star_catalogue.tex Outdated Show resolved Hide resolved
guide/app_star_catalogue.tex Outdated Show resolved Hide resolved
@gzotti
Copy link
Member

gzotti commented Nov 7, 2021

In the older list you had approx. year ranges for what I think are the classical "catalogs". Probably you can add these years back, where available. There will be no new Struwe numbers, but OCC may still be added.

@alex-w alex-w marked this pull request as draft November 9, 2021 18:14
@alex-w alex-w marked this pull request as ready for review November 11, 2021 17:23
@github-actions github-actions bot requested a review from gzotti November 11, 2021 17:23
@alex-w alex-w requested a review from gzotti November 11, 2021 20:03
@alex-w
Copy link
Member Author

alex-w commented Nov 11, 2021

Comment to the SUG: the section about designation of double stars maybe improve, but only after reading the special literature.

@gzotti
Copy link
Member

gzotti commented Nov 11, 2021

Great list! Testing a bit...

@gzotti
Copy link
Member

gzotti commented Nov 11, 2021

Fine for me!

@alex-w alex-w merged commit cf52aa7 into master Nov 11, 2021
@alex-w alex-w deleted the refactoring/stars-designations branch November 11, 2021 21:45
GUI automation moved this from In progress to Done Nov 11, 2021
@alex-w alex-w added the state: fixed The bug has been fixed label Nov 14, 2021
@github-actions
Copy link

Hello @alex-w! 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: fixed The bug has been fixed label Dec 25, 2021
@github-actions
Copy link

Hello @alex-w! 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
enhancement Improve existing functionality
Projects
GUI
  
Done
Development

Successfully merging this pull request may close these issues.

Invisibe labels of stars
2 participants