StelCore: reduce confusion around location signals

[gzotti]

Description

StelCore has two signals after a location change.

• locationChanged(loc)
• targetLocationChanged(loc, landscapeID)

Those were likely introduced with different intentions in mind, but now are both emitted. Their documentation was lame.
Several other components are connected to the signals or emit them by themselves, with possible side effects.
See #3173 and see difficulties around the "best eclipse sites".

IMHO this area of location settings needs to be checked thouroughly and slimmed down or at least made more intuitive.

These points are to be considered

• Removed default value in signal, force default second arg.
• Emit only one of the two in StelCore::moveObserverTo() EDIT: Reverted. They have both their uses.
• Clicking on the map sets a "zero" landscape with color taken from the clicked point in the map. This is limited to terrestrial locations. For other planets, a "planet" landscape is always used.
• if default planet landscape cannot be found, a gray zero landscape seems useful

Fixes: #3173 and other possible signal loops or unexpected results or non-results.

I invite @alex-w and @10110111 to collaborate on that.

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

How Has This Been Tested?

Test Configuration:

• Operating system: Win11
• 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

[github-actions]

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]

locationChanged() uses in many places - SolarSystem, AstroCalc, ArchaeoLines, Meteor Showers, Navigational Stars, Observability, Satellites, etc.

By the fact targetLocationChanged() is newly introduced feature for manage changes in LandscapeMgr & Location dialog + RemoteSync plugin

[gzotti]

Do I understand that targetLocationChanged is "next location to be set, but with a transition animation". This means, LandscapeMgr should receive targetLocation to prepare a new landscape, but without changing location. And the actual location switch should not trigger other things. Currently the RemoteSync landscape switch seems to fail.

[alex-w]

Do I understand that targetLocationChanged is "next location to be set, but with a transition animation". This means, LandscapeMgr should receive targetLocation to prepare a new landscape, but without changing location. And the actual location switch should not trigger other things. Currently the RemoteSync landscape switch seems to fail.

Broke everything except one feature is not a good solution :) In Location dialog targetLocationChanged can be replaced by locationChanged without problem.

[gzotti]

We are breaking things in circles without knowing. Somebody fixes something, breaking others in the way. That needs a bit more study, and documentation of developers intents and observations. Not "not a good solution", but "not a good solution because...". Also, if a comment was "NOTE: Use ...", the next "fix" should not break what is written in the note and remove it.

For example, probably the SpaceShipObserver::update(double deltaTime) should NOT set a new landscape when it has reached its target, when that has been done already because LandscapeMgr has received the targetLocationChanged signal. But maybe it is required because some other effect would else be missing. Still, landscape should probably be switched only once where needed.

[gzotti]

I now try 10s transitions for the solar eclipses (1-2s would be good for a soft move, this is just for demonstration). They are really bad, at around 1fps. Are there just too many computations (RTS, local circumstances, always requiring lots of extra computations...), or is there still some signal loop?
In the first case, using immediate view is the only useful way...

[gzotti]

D'oh! Found while testing #3187... I had a huge ssystem_minor.ini. With default file, it is just as I wanted it! *-/

Probably when auto-moving between locations, updating SolarSystem or at least MinorPlanets and Comets should be inhibited. It's usually just for a few seconds.

[alex-w]

Great! I forget about Qt::UniqueConnection for signal/slot

[gzotti]

Seems we need a fallback to "zero" landscape when going e.g. to Venus (as unattractive as that location may be) or other sites with no default planet landscape.

[gzotti]

In the Config/Tools tab we have "Auto-enabling for the environment" and "auto select landscapes". These may be better in the Location dialog or in the View/Landscape tab. Also, one sets landscape, the other both landscape and atmosphere. Seems somewhat redundant.

[alex-w]

In the Config/Tools tab we have "Auto-enabling for the environment" and "auto select landscapes". These may be better in the Location dialog or in the View/Landscape tab. Also, one sets landscape, the other both landscape and atmosphere. Seems somewhat redundant.

Probably splitting onto 2 options - to follow atmosphere and landscape settings from the planet - and moving both into Location dialog seems acceptable to avoid misunderstandings

[github-actions]

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

[gzotti]

The auto-colored zero horizon works in the location dialog. Clicking e.g. into the desert now shows a sandy color sampled from the map. However, selecting best place from the solar eclipse list now shows the default green zero landscape. I could add a general coordinate->color lookup to StelLocationMgr. Before I continue, @alex-w , @10110111, @worachate001, do you agree on this feature? Should then any location change switch to zero+mapColor? (only for "flagLandscapeAutoSelection"). Probably even when storing zero as default landscape, it could then be re-colored on startup (or color stored in config.ini?). To continue on that, this could be extended to a LandscapeMgr.PolygonalLandscapeColor property that is applied to all polygonal landscapes. Maybe in the next round, however.

[10110111]

Just tried going to the best point of a solar eclipse, and both atmosphere and ground were disabled. I enabled them, chose another eclipse, but again they get disabled. The same happens when I click on the map.

If I enable atmosphere and ground and save settings, then they don't get disabled on location switch.

Generally, the choice of color from the map sounds good. Though, if we appear in the ocean, the landscape could better be "Ocean", rather than "Zero Horizon".

[gzotti]

Have you enabled both "Auto-enable environment" and esp. "Auto select landscapes"? I must check (and probably fix) behaviour with these settings disabled. These switches should also move into the LocationDialog.

The problem about using ocean is that the color of the ocean in our map is not singular, and the dark-blue may be used in small interior lakes as well. See the bright-blue Caribbean sea. If we had another 1-bit map or 4-bit land characteristics map, we could develop/define "typical" landscape panos with as close-to-zero as possible skylines. These could be used for flat snow fields and open grassland. But problems are of course mountaineous landscapes or forested areas... this is beyond the current PR.

Another coordinate-based lookup could define time zones, but this may have to change occasionally.

Another way to extend the location map would be a zooming option. I think you have added the larger map image. This is never fully utilized. Or we extend with the latest Qt feature, https://doc.qt.io/qt-6/qml-location5-maps.html. This could start as optional plugin on Qt6.5+. The map feature must remain available also for offline use of course.

[10110111]

Have you enabled both "Auto-enable environment" and esp. "Auto select landscapes"?

• Auto-enabling for the environment
• Auto-select landscapes

[10110111]

When clicking the map, I get the following message in the log:

QImage::scaled: Image is a null image
QImage::pixelColor: coordinate (0,0) out of range

And the Zero Horizon auto-selected landscape is black. This is on Qt 6.

[10110111]

I also get this warning at the loading time:

qt.core.qobject.connect: QObject::connect: No such signal StelCore::StelCore::locationChanged(StelLocation)
qt.core.qobject.connect: QObject::connect:  (sender name:   'StelCore')

Full log: log.txt.

[Atque]

And the Zero Horizon auto-selected landscape is black. This is on Qt 6.

Huh? It works on Windows 10 64-bit, Qt 6.4.2.

[10110111]

Huh? It works on Windows 10 64-bit, Qt 6.4.2.

Maybe StelFileMgr::findFile somehow finds the file in your source tree. At least on Linux the installation prefix doesn't contain miscWorldMap.jpg at all.

[gzotti]

Yes, I usually run my test builds from the source dir (probably @Atque does the same), so these files are found.

[10110111]

I also get this warning at the loading time

This warning originates from StelLocationMgr::StelLocationMgr(). Why do we even use string-based connect here? The warning is gone if I switch to the pointer-based connect.

[gzotti]

The connect(sender, SIGNAL(...), receiver, SLOT(...)) syntax is used almost everywhere simply by tradition.

[10110111]

almost everywhere simply by tradition

This should change just as Q_OVERRIDE tradition was changed.

[alex-w]

Did you check with the checkbox unchecked?

Yes

[10110111]

/home/ruslan/src/stellarium/src/core/StelLocationMgr.cpp: In member function ‘void StelLocationMgr::changePlanetMapForLocation(StelLocation)’:
/home/ruslan/src/stellarium/src/core/StelLocationMgr.cpp:1578:45: error: expected primary-expression before ‘)’ token
 1578 |   SolarSystem *ssm=GETSTELMODULE(SolarSystem);
      |                                             ^
/home/ruslan/src/stellarium/src/core/StelLocationMgr.cpp:1578:20: error: ‘GETSTELMODULE’ was not declared in this scope
 1578 |   SolarSystem *ssm=GETSTELMODULE(SolarSystem);
      |                    ^~~~~~~~~~~~~

[alex-w]

/home/ruslan/src/stellarium/src/core/StelLocationMgr.cpp: In member function ‘void StelLocationMgr::changePlanetMapForLocation(StelLocation)’:
/home/ruslan/src/stellarium/src/core/StelLocationMgr.cpp:1578:45: error: expected primary-expression before ‘)’ token
1578 | SolarSystem *ssm=GETSTELMODULE(SolarSystem);
| ^
/home/ruslan/src/stellarium/src/core/StelLocationMgr.cpp:1578:20: error: ‘GETSTELMODULE’ was not declared in this scope
1578 | SolarSystem *ssm=GETSTELMODULE(SolarSystem);
| ^~~~~~~~~~~~~

Missing include, let me check

[gzotti]

Sorry, I seem to have missed a few includes because of PCH.

[github-actions]

Hello @gzotti!

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

[github-actions]

Hello @gzotti!

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