Expansion of skycultures through spatial and temporal extent

[mRaetz]

Description

This extension adds spatial and temporal dimensions to sky cultures. These are used to allow users to make interactive selections. The selection menu has been redesigned for this purpose. In addition to a list, users can now explore and select the available cultures using a time-resolved map. All new cultures in the future should have both a temporal and spatial dimension, which is why the SkyCultureMaker plugin has been updated as well. In addition to a region selection feature, a simple digitization tool has been implemented that allows users to digitize the culture territories themselves. Furthermore, a few minor changes were made that could be described as bug fixes. For example, a problem with pressing the ESC key in ScmSkyCultureDialog was fixed, as well as the previously invisible button for the artwork tooltips in ScmConstellationDialog.

• addition of spatial of temporal dimension to skyculture def
• new menu for skyculture selection that allows exploration of the available cultures
• integration of spatial / temporal extension into the SkyCultureMaker plugin
• small changes / bugfixes to the SkyCultureMaker plugin

Type of change

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

How Has This Been Tested?

This extension has been manually tested on a desktop PC (Windows 10) and a laptop (Windows 11). In both cases, there were no compatibility issues. However, no statement can be made regarding correct functionality on other operating systems.

Test specification:

• OS: Windows 10 / 11
• CPU: AMD Ryzen 7 2700x
• GPU: NVIDIA GeForce GTX 1660 Ti

Checklist:

• [x] My code follows the code style of this project.
• [x] I have performed a self-review of my own code
• [x] 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
• [x] My changes generate no new warnings
• [ ] I have added tests that prove my fix is effective or that my feature works
• [x] New and existing unit tests pass locally with my changes
• [ ] Any dependent changes have been merged and published in downstream modules

[github-actions[bot]]

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

[alex-w]

Oops... @mRaetz please resolve conflicts

[github-actions[bot]]

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

[xLPMG]

I compiled this on my machine and your code generates a few new warnings that need to be fixed. Some examples include

ScmSkyCultureDialog.cpp:659:69: warning: unused parameter 'column' [-Wunused-parameter]
  659 | void ScmSkyCultureDialog::selectLocation(QTreeWidgetItem *item, int column)
      |                                                                     ^
ScmMultiselectionComboBox.cpp:131:12: warning: variable 'currentModel' set but not used [-Wunused-but-set-variable]
  131 |         if (auto *currentModel = model)
      |                   ^
SkycultureMapGraphicsView.cpp:452:8: warning: unused variable 'maxDuration' [-Wunused-variable]
  452 |         qreal maxDuration = 2000;
      |               ^~~~~~~~~~~
SkyculturePolygonItem.cpp:30:4: warning: field 'isHovered' will be initialized after field 'lastSelectedState' [-Wreorder-ctor]
   30 |         , isHovered(false)
      |           ^~~~~~~~~~~~~~~~
      |           lastSelectedState(false)
   31 |         , lastSelectedState(false)
      |           ~~~~~~~~~~~~~~~~~~~~~~~~
      |           isHovered(false)

If these dont appear in your console, maybe try deleting the build folder and doing a fresh build.

Also now that I see this warning: the usual name scheme was to use camel case for "SkyCulture", but your files use "Skyculture". Its only a cosmetic issue, but it would still be nice if the same naming scheme is used everywhere :))

[mRaetz]

Thanks for the tip and for pointing out the incorrect names. I didn't even notice the mistake with the lowercase c.

Another thing i noticed when looking over the files is that one of the buttons in ScmHideOrAbortMakerDialog is incorrectly translated in german. The buttons are supposed to be distinguishable (hide - abort - cancel) but in the current version they are not (Ausblenden - Abbrechen - Abbrechen)

I'm also not sure how to solve the problem with the SvgWidgets module. Apparently, there was a change in Qt in this regard, but even if there is a difference between Qt5 and Qt6, I don't understand why the checks for Linux (amd64; qt6; core) / Linux (amd64; qt6) failed when they passed for Windows and macOS with qt6.

[alex-w]

I'm also not sure how to solve the problem with the SvgWidgets module. Apparently, there was a change in Qt in this regard, but even if there is a difference between Qt5 and Qt6, I don't understand why the checks for Linux (amd64; qt6; core) / Linux (amd64; qt6) failed when they passed for Windows and macOS with qt6.

Please add missing package in linux boxes

[10110111]

The buttons are supposed to be distinguishable (abort - hide - cancel) but in the current version they are not (Ausblenden - Abbrechen - Abbrechen)

Actually, even in English "Abort" and "Cancel" require too much consideration to figure out what one should press. What if we change them to more explicit "Drop progress and close" and "Don't close"?

The current situation reminds me of the MSVC's age-old "Debug error!" dialog box with the stock "Abort", "Retry", "Ignore" buttons and the "(Press Retry to debug the application)" kludge instead of having sensible button labels.

[10110111]

I had to apply the following patch to build without precompiled headers on Ubuntu 20.04 and Qt 6.4.1:

diff --git a/src/gui/SkyCultureMapGraphicsView.cpp b/src/gui/SkyCultureMapGraphicsView.cpp
index c5fcee207a..edaa5faaec 100644
--- a/src/gui/SkyCultureMapGraphicsView.cpp
+++ b/src/gui/SkyCultureMapGraphicsView.cpp
@@ -20,6 +20,7 @@
 #include "SkyCultureMapGraphicsView.hpp"
 #include "SkyCulturePolygonItem.hpp"
 #include "StelLocaleMgr.hpp"
+#include "StelFileMgr.hpp"
 #include "StelSkyCultureMgr.hpp"
 #include <qjsonarray.h>
 #include <QGraphicsSvgItem>
@@ -27,7 +28,10 @@
 
 #include <QJsonObject>
 #include <QJsonDocument>
+#include <QWheelEvent>
+#include <QMouseEvent>
 #include <QtMath>
+#include <QFile>
 
 SkyCultureMapGraphicsView::SkyCultureMapGraphicsView(QWidget *parent)
     : QGraphicsView(parent)

[10110111]

So, I've finally tried this PR. Here's my critique:

0. See the above comment.
1. The separate "Sky Culture Description" looks quite out of place as a category among the other tabs. Would it be more appropriate to make it a subtab inside the "Sky culture" tab?
2. The labels like "current Time", "min Year" etc. should be capitalized in a way to match the rest of the GUI, i.e. "Current time", "Minimum year" etc. (note "minimum" instead of "min", because there's no reason to shorten it, since we have plenty of space in this row).
3. Why does the "current Time" spinbox look so different from all the other spinboxes? The -/+ buttons seem to do exactly the same as the usual down/up buttons.
4. The "Enter Culture..." filter behaves strangely: I type b, it shows all SCs that contain a letter "b" in their names, but if I type be, almost all SCs reappear (but some are still filtered out).
5. I can move the world map by half its size up and down, showing useless space above or below, which makes no sense to me.
6. Sky Culture Description contains most of the settings, which is not obvious from tab name. This implies that the name should rather be something like "Sky Culture Details".

[github-actions[bot]]

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

[mRaetz]

The separate "Sky Culture Description" looks quite out of place as a category among the other tabs. Would it be more appropriate to make it a subtab inside the "Sky culture" tab?

Originally, the map and description were in the same tab (a stackedWidget allowed users to switch between them). On the suggestion of GZotti / SMH, the description was later moved to this extra tab.

Sky Culture Description contains most of the settings, which is not obvious from tab name. This implies that the name should rather be something like "Sky Culture Details".

Yes, the name could definitely be better. If the description stays in its own tab, I would also prefer this name.

Why does the "current Time" spinbox look so different from all the other spinboxes? The -/+ buttons seem to do exactly the same as the usual down/up buttons.

This was purely a design decision. I felt that buttons on the left and right would better convey the connection with the horizontal slider. The normal up/down buttons rotated and placed on the left/right looked a bit strange, which is why I used -/+ buttons. If this disturbs the overall design too much, the normal up/down buttons on the right side of the spin box can be used instead.

The "Enter Culture..." filter behaves strangely: I type b, it shows all SCs that contain a letter "b" in their names, but if I type be, almost all SCs reappear (but some are still filtered out).

Yes, that's right. I initially used a strict query (toLower(filterString) must be contained in the word). While testing, I noticed that some cultures are difficult to find with this query. Typos are an obvious problem, but special characters can also prevent the filter from matching the word. A concrete example of this is as follows: In the German version, the indigenous culture of New Zealand is referred to as Māori. However, the ā is a very rare character, so it must be assumed that the user will enter Maori (normal a) in the search bar. Therefore, I tried to use a kind of string similarity measure to solve this problem. This is not perfect, but most importantly, it does not filter individual characters (a deviation of at least one character should be allowed to avoid the problems described above). Therefore, it is only used for 2 or more characters, which leads to the behavior described. I am sure there is a better solution for the whole thing, but the current implementation has basically done its job. However, I would also be open to using a simple, strict matching (via contains).

I can move the world map by half its size up and down, showing useless space above or below, which makes no sense to me.

If the scene is the same size as the map, navigation of the map becomes restricted. On the one hand, you could then only view the edge of the map (depending on the zoom level) at the edge of the window, but most of the time you want the relevant part to be in the center of your field of view. On the other hand, there is a problem with smooth zooming after selecting a culture from the list. Since during this zoom only the scene is moved to create the illusion of smooth movement, it occurs, especially in the border areas of the map, that you reach the boundary of the scene, resulting in a rigid movement. This can be solved by setting the extent of the scene to a multiple of the map.

[10110111]

This was purely a design decision. I felt that buttons on the left and right would better convey the connection with the horizontal slider.

I don't think it's a good idea, because it makes widget style inconsistent and makes it seem that the two spinboxes near each other are actually different kinds of widgets. We already have similar combinations of a slider and a (normal) spinbox, and they don't cause any confusion. See e.g. Labels and Markers in the DSO tab of the View dialog, or Shooting stars in the Sky tab.

Typos are an obvious problem, but special characters can also prevent the filter from matching the word.

You could strip diacritics from both the search term and the list of names before doing the filtering. See e.g. here on how to do it.

most of the time you want the relevant part to be in the center of your field of view

Even if we go this way, it shouldn't be possible to move the edge of the map farther away than to the center of the window. Currently I can move the whole map away when scaling up so as to fit Greenland in the window, and may have to pan several window heights to get the map back into the viewport.