Skip to content

Multiple label options for Skycultures #4179

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

Open
wants to merge 62 commits into
base: master
Choose a base branch
from
Open

Multiple label options for Skycultures #4179

wants to merge 62 commits into from

Conversation

gzotti
Copy link
Member

@gzotti gzotti commented Mar 6, 2025

Description

This is a step forward in the Skycultures expansion. Given various new naming components of which only a few tags may exist per skyculture, this should allow configuring user preferences, per skyculture, of how labels are to be configured from nativeName, pronunciation, translated name etc.

Fundamental is the introduction of StelObject::CulturalDisplayStyle, which allows cultural/linguistic awareness in all StelObjects. StelObject receives two new methods, getScreenLabel() and getInfoLabel(), which format the respective labels from the various components.

Done

  • Label enum in StelObject, properties in StelSkyCultureMgr, GUI Switch, property storage
  • Constellations
  • Asterisms
  • Star names: Do we need yet another different setting?
  • Planet names: Do we need yet another different setting?
  • DSO names: Do we need yet another different setting?

Fixes # (issue)

Screenshots (if appropriate):

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

How Has This Been Tested?

Test Configuration:

  • Operating system: Win10/11
  • Graphics Card: irrelevant

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

@gzotti gzotti added feature Entirely new feature importance: high Obvious error, very annoying, but no crash subsystem: skycultures The issue is related to skycultures of planetarium... purpose: cultural astronomy Issues, pull requests and proposals with cultural astronomy purposes labels Mar 6, 2025
@gzotti gzotti added this to the 25.2 milestone Mar 6, 2025
@gzotti gzotti self-assigned this Mar 6, 2025
@github-actions github-actions bot requested review from 10110111 and alex-w March 6, 2025 17:34
Copy link

github-actions bot commented Mar 6, 2025

Hello @gzotti!

Thank you for proposing of the feature.

Copy link

github-actions bot commented Mar 6, 2025

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?

Files matching skycultures/**:

  • Did you remember to update skycultures/CMakeLists.txt file respectively to changes in sky cultures?
  • Did you remember to define classification parameter in sky cultures (see index.json file)?
  • Did you remember to define license parameter in sky cultures (see description.md file)?
  • Did you remember to define region parameter in sky culture (see index.json file)?

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

Copy link

github-actions bot commented Mar 7, 2025

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the has conflicts The pull request has conflicts label Mar 7, 2025
@alex-w alex-w linked an issue Mar 22, 2025 that may be closed by this pull request
3 tasks
Copy link

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot removed the has conflicts The pull request has conflicts label Mar 24, 2025
@gzotti
Copy link
Member Author

gzotti commented Mar 29, 2025

When @10110111 was working on the big format switch, he asked about the value of separate translators for program GUI and skycultures. IIRC we tried to circumvent limitations of the old data files. Please @alex-w, can you share some insights? We now have native names, transliteration/pronounciation etc., which should all make the native names understandable to the user in his own language. Do we still need two language settings?

The current state has the new data items available for constellations, asterisms, planets and DSO. Not polished yet, as the old solutions are still partially in use and could likely be simplified a bit. Class members which can have more than one cultural name own a list of CulturalNames (to never mix elements) from which the first is used as screen label, and all are visible in the Infostring label. Stars should come early next week, and I may have to extend the output options to include the "modern" name in a few more combinations, but I think the general direction is OK.

In StarMgr, what does loadCultureSpecificNameForNamedObject()? The non-stars are handled by SolarSystem and NebulaMgr, so what is that for?

@10110111
Copy link
Contributor

In StarMgr, what does loadCultureSpecificNameForNamedObject()? The non-stars are handled by SolarSystem and NebulaMgr, so what is that for?

It's for supplying names for stars specified by name, it uses the database from skycultures/common_star_names.fab.

@gzotti
Copy link
Member Author

gzotti commented Mar 30, 2025

Ah, OK. But I see it's never used, actually, and is a "just in case"? The only names following NAME so far are Planet names, at least for the SCs in master. DSO like M45 are labeled directly. Can we not just force SC authors to use HIP/Gaia designations directly (not a very high demand, right?) to simplify the code (i.e., remove that function)?

@10110111
Copy link
Contributor

But I see it's never used, actually, and is a "just in case"?

This is in use in some SCs in the SC repo, e.g. Anutan.

Can we not just force SC authors to use HIP/Gaia designations directly (not a very high demand, right?) to simplify the code (i.e., remove that function)?

I'm afraid not. I'd be glad to do this (and in my view we should do it), but it seems Fabien wants to extend this functionality even further, to allow not just names, but also things like "lines": [["* gam Cru", "* del Cru"]],, which will require some additional processing.

@gzotti
Copy link
Member Author

gzotti commented Mar 30, 2025

But I see it's never used, actually, and is a "just in case"?

This is in use in some SCs in the SC repo, e.g. Anutan.

Ah, like that. Hmm, OK. Still, I regard this as unneeded burden for software maintenance. But if it's used now, I'll comply.

Can we not just force SC authors to use HIP/Gaia designations directly (not a very high demand, right?) to simplify the code (i.e., remove that function)?

I'm afraid not. I'd be glad to do this (and in my view we should do it), but it seems Fabien wants to extend this functionality even further, to allow not just names, but also things like "lines": [["* gam Cru", "* del Cru"]],, which will require some additional processing.

I honestly don't see the necessity for that. All stars are meanwhile addressable by HIP or Gaia numbers.

@gzotti
Copy link
Member Author

gzotti commented Mar 31, 2025

To get a bit of clarity around the not or wrongly documented entries:

StarsMgr::commonNamesMap: This is the list from skycultures/common_star_names.fab, but can be modified when loading a skyculture. However, SC names now have several more components, I think this should not be mixed.

Is the StarsMgr::additionalNamesMap and additionalNamesMapI18n exclusively reserved for the Cultural names? If yes, I will replace by a QMap<starId, QList>

@10110111
Copy link
Contributor

10110111 commented Apr 1, 2025

Actually, I was a bit wrong about this. The names from skycultures/common_star_names.fab only get into commonNamesMap when skyCulture.fallbackToInternationalNames is set. When this happens, these names are added first. After this the logic with commonNamesMap, commonNamesMapI18n etc. is the same as before the switch to the new format.

The names used for finding stars specified in index.json by name are assigned to commonNamesIndexToSearchWhileLoading local variable in StarMgr::updateSkyCulture and this is the only index used for searching.

@gzotti
Copy link
Member Author

gzotti commented Apr 1, 2025

More questions on code quality in StarMgr, @alex-w : The lists doubleHipStars, variableHipStars, algolTypeStars, classicalCepheidsTypeStars, hipStarsHighPM are lists of 1-element QMaps. Is that really intended? Why not just QMaps? Or lists of QPairs? They are used in Astrocalc, but in a very simple way. You can iterate over QMaps as well.

@alex-w
Copy link
Member

alex-w commented Apr 1, 2025

These lists using in AstroCalc and Search tools. I didn’t use QMap to not mixed iteration QList/QMap elements (though it was added before refactoring AstroCalc).

@alex-w
Copy link
Member

alex-w commented Apr 1, 2025

Please @alex-w, can you share some insights? We now have native names, transliteration/pronounciation etc., which should all make the native names understandable to the user in his own language. Do we still need two language settings?

Probably we can remove second dropdown list for languages (skycultures) after introduction the support of extra options for labels. At least I don’t see critical necessary for second list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Entirely new feature importance: high Obvious error, very annoying, but no crash purpose: cultural astronomy Issues, pull requests and proposals with cultural astronomy purposes subsystem: skycultures The issue is related to skycultures of planetarium...
4 participants