76 bug save a constellation results in fatal assert on windows (#86)

* fix: assertion + perf: less value copies

The assertion was a result because the SPainter was passed by value i.e. copied and not passed by reference

* chore: merge mistake from main stellarium

* fix: merge mistakes

* chore: improved function signatures

* fix: update error handling

* chore: cleanup

* fix: label not drawn

Author: RivinHD

CMakeLists.txt

@@ -307,11 +307,11 @@ IF(WIN32)
      IF(NOT MSVC)
           # MinGW requires enabling of exceptions
           SET(STEL_MINGW_FLAGS "-fexceptions")
-          # -Og and -s removes symbols an relocation information as otherwise the ordinal gets out of maximum 16-bit range.
+          # -Og removes some symbols as otherwise the ordinal exceeds the maximum of the 16-bit range.
           SET(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -Og") 
-          # MinGW clang compiler does not have a these flag, but MinGW gcc requires MinGW-specific threading, version number storage
+          # MinGW clang compiler does not have these flags, but MinGW gcc requires MinGW-specific threading, version number storage
           IF(NOT ("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang"))
-               SET(STEL_MINGW_FLAGS "${STEL_MINGW_FLAGS} -fident -mthreads -s")
+               SET(STEL_MINGW_FLAGS "${STEL_MINGW_FLAGS} -fident -mthreads")
           ENDIF()
           SET(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${STEL_GCC_C_FLAGS} ${STEL_MINGW_FLAGS}")
           SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${STEL_GCC_CXX_FLAGS} ${STEL_MINGW_FLAGS}")

plugins/SkyCultureMaker/src/ScmAsterism.cpp

@@ -1,22 +1,21 @@
 #include "ScmAsterism.hpp"
 
-void scm::ScmAsterism::setCommonName(ScmCommonName name)
+void scm::ScmAsterism::setCommonName(const ScmCommonName &name)
 {
-    commonName = name;
+	commonName = name;
 }
 
-void scm::ScmAsterism::setId(QString id)
+void scm::ScmAsterism::setId(const QString &id)
 {
-    ScmAsterism::id = id;
+	ScmAsterism::id = id;
 }
 
 scm::ScmCommonName scm::ScmAsterism::getCommonName() const
 {
-	return commonName; 
+	return commonName;
 }
 
-
 QString scm::ScmAsterism::getId() const
 {
-    return id;
+	return id;
 }

plugins/SkyCultureMaker/src/ScmAsterism.hpp

@@ -9,54 +9,52 @@
 
 #ifndef SCM_ASTERISM_HPP
 #define SCM_ASTERISM_HPP
- 
-#include "ScmCommonName.hpp"
+
 #include "QString"
- 
+#include "ScmCommonName.hpp"
+
 namespace scm
 {
- 
+
 class ScmAsterism
 {
 public:
-    /**
+	/**
     * @brief Sets the id of the asterism
     * 
     * @param id id
     */
-    void setId(QString id);
+	void setId(const QString &id);
 
-    /**
+	/**
     * @brief Gets the id of the asterism
     * 
     * @return id
     */
-    QString getId() const;
+	QString getId() const;
 
-    /**
+	/**
      * @brief Sets the common name of the asterism.
      *
      * @param name The common name of this asterim.
      */
-    void setCommonName(ScmCommonName name);
+	void setCommonName(const ScmCommonName &name);
 
-    /**
+	/**
      * @brief Returns the common name of the asterism.
      *
      * @return The common name of the this asterism.
      */
-    ScmCommonName getCommonName() const;
- 
+	ScmCommonName getCommonName() const;
+
 private:
-    /// Id of the Asterism
-    QString id;
+	/// Id of the Asterism
+	QString id;
 
-    /// Common name of the constellation
-    ScmCommonName commonName;
+	/// Common name of the constellation
+	ScmCommonName commonName;
 };
 
-}  // namespace scm
+} // namespace scm
 
- 
-#endif	// SCM_ASTERISM_HPP
- 
+#endif // SCM_ASTERISM_HPP

plugins/SkyCultureMaker/src/ScmCommonName.cpp

@@ -1,31 +1,31 @@
 #include "ScmCommonName.hpp"
 
-scm::ScmCommonName::ScmCommonName(QString id)
+scm::ScmCommonName::ScmCommonName(const QString &id)
 {
 	ScmCommonName::id = id;
 }
 
-void scm::ScmCommonName::setEnglishName(QString name)
+void scm::ScmCommonName::setEnglishName(const QString &name)
 {
 	englishName = name;
 }
 
-void scm::ScmCommonName::setNativeName(QString name)
+void scm::ScmCommonName::setNativeName(const QString &name)
 {
 	nativeName = name;
 }
 
-void scm::ScmCommonName::setPronounce(QString pronounce)
+void scm::ScmCommonName::setPronounce(const QString &pronounce)
 {
 	ScmCommonName::pronounce = pronounce;
 }
 
-void scm::ScmCommonName::setIpa(QString ipa)
+void scm::ScmCommonName::setIpa(const QString &ipa)
 {
 	ScmCommonName::ipa = ipa;
 }
 
-void scm::ScmCommonName::setReferences(std::vector<int> refs)
+void scm::ScmCommonName::setReferences(const std::vector<int> &refs)
 {
 	references = refs;
 }

plugins/SkyCultureMaker/src/ScmCommonName.hpp

@@ -10,33 +10,33 @@
 #ifndef SCM_COMMONNAME_HPP
 #define SCM_COMMONNAME_HPP
 
-#include <QString>
+#include <optional>
 #include <vector>
 #include <QJsonObject>
-#include <optional>
+#include <QString>
 
 namespace scm
 {
 
 class ScmCommonName
 {
 public:
-	ScmCommonName(QString id);
+	ScmCommonName(const QString &id);
 
 	/// Sets the english name
-	void setEnglishName(QString name);
+	void setEnglishName(const QString &name);
 
 	/// Sets the native name
-	void setNativeName(QString name);
+	void setNativeName(const QString &name);
 
 	/// Sets the native name in European glyphs or Pinyin for Chinese.
-	void setPronounce(QString pronounce);
+	void setPronounce(const QString &pronounce);
 
 	/// Sets the native name in IPA (International Phonetic Alphabet)
-	void setIpa(QString name);
+	void setIpa(const QString &name);
 
 	/// Sets the references to the sources of the name spellings
-	void setReferences(std::vector<int> refs);
+	void setReferences(const std::vector<int> &refs);
 
 	/// Returns the english name
 	QString getEnglishName() const;
@@ -80,6 +80,6 @@ class ScmCommonName
 	std::optional<std::vector<int>> references;
 };
 
-}  // namespace scm
+} // namespace scm
 
-#endif	// SCM_COMMONNAME_HPP
+#endif // SCM_COMMONNAME_HPP

plugins/SkyCultureMaker/src/ScmConstellation.cpp

@@ -1,18 +1,21 @@
 #include "ScmConstellation.hpp"
 
-scm::ScmConstellation::ScmConstellation(std::vector<scm::CoordinateLine> coordinates, std::vector<scm::StarLine> stars)
+scm::ScmConstellation::ScmConstellation(const std::vector<scm::CoordinateLine> &coordinates,
+                                        const std::vector<scm::StarLine> &stars)
 	: constellationCoordinates(coordinates)
 	, constellationStars(stars)
 {
-	QSettings* conf = StelApp::getInstance().getSettings();
+	QSettings *conf = StelApp::getInstance().getSettings();
 	constellationLabelFont.setPixelSize(conf->value("viewing/constellation_font_size", 15).toInt());
- 
+
 	QString defaultColor = conf->value("color/default_color", "0.5,0.5,0.7").toString();
-	colorDrawDefault = Vec3f(conf->value("color/const_lines_color", defaultColor).toString());
-	colorLabelDefault = Vec3f(conf->value("color/const_names_color", defaultColor).toString());
+	colorDrawDefault     = Vec3f(conf->value("color/const_lines_color", defaultColor).toString());
+	colorLabelDefault    = Vec3f(conf->value("color/const_names_color", defaultColor).toString());
+
+	updateTextPosition();
 }
 
-void scm::ScmConstellation::setId(QString id)
+void scm::ScmConstellation::setId(const QString &id)
 {
 	ScmConstellation::id = id;
 }
@@ -22,7 +25,7 @@ QString scm::ScmConstellation::getId() const
 	return id;
 }
 
-void scm::ScmConstellation::setEnglishName(QString name)
+void scm::ScmConstellation::setEnglishName(const QString &name)
 {
 	englishName = name;
 }
@@ -32,56 +35,54 @@ QString scm::ScmConstellation::getEnglishName() const
 	return englishName;
 }
 
-void scm::ScmConstellation::setNativeName(std::optional<QString> name)
+void scm::ScmConstellation::setNativeName(const std::optional<QString> &name)
 {
 	nativeName = name;
 }
 
-void scm::ScmConstellation::setPronounce(std::optional<QString> pronounce)
+void scm::ScmConstellation::setPronounce(const std::optional<QString> &pronounce)
 {
 	ScmConstellation::pronounce = pronounce;
 }
 
-void scm::ScmConstellation::setIPA(std::optional<QString> ipa)
+void scm::ScmConstellation::setIPA(const std::optional<QString> &ipa)
 {
 	ScmConstellation::ipa = ipa;
 }
 
-void scm::ScmConstellation::setConstellation(std::vector<CoordinateLine> coordinates, std::vector<StarLine> stars)
+void scm::ScmConstellation::setConstellation(const std::vector<CoordinateLine> &coordinates,
+                                             const std::vector<StarLine> &stars)
 {
 	constellationCoordinates = coordinates;
-	constellationStars = stars;
+	constellationStars       = stars;
+
+	updateTextPosition();
 }
 
-void scm::ScmConstellation::drawConstellation(StelCore *core, Vec3f color)
+void scm::ScmConstellation::drawConstellation(StelCore *core, const Vec3f &color) const
 {
 	StelPainter painter(core->getProjection(drawFrame));
 	painter.setBlending(true);
 	painter.setLineSmooth(true);
 	painter.setFont(constellationLabelFont);
-	
+
 	bool alpha = 1.0f;
 	painter.setColor(color, alpha);
 
-	XYZname.set(0.,0.,0.);
 	for (CoordinateLine p : constellationCoordinates)
 	{
 		painter.drawGreatCircleArc(p.start, p.end);
-		XYZname += p.end;
-		XYZname += p.start;
 	}
 
-	XYZname.normalize();
-
-	drawNames(core, painter, colorLabelDefault);
+	drawNames(core, painter);
 }
 
-void scm::ScmConstellation::drawConstellation(StelCore *core)
+void scm::ScmConstellation::drawConstellation(StelCore *core) const
 {
 	drawConstellation(core, colorDrawDefault);
 }
 
-void scm::ScmConstellation::drawNames(StelCore *core, StelPainter sPainter, Vec3f labelColor) 
+void scm::ScmConstellation::drawNames(StelCore *core, StelPainter &sPainter, const Vec3f &labelColor) const
 {
 	sPainter.setBlending(true);
 
@@ -91,26 +92,27 @@ void scm::ScmConstellation::drawNames(StelCore *core, StelPainter sPainter, Vec3
 		velocityObserver = core->getAberrationVec(core->getJDE());
 	}
 
-	XYZname+=velocityObserver;
-	XYZname.normalize();
+	Vec3d namePose = XYZname;
+	namePose += velocityObserver;
+	namePose.normalize();
 
-	if(!sPainter.getProjector()->projectCheck(XYZname, this->XYname))
+	Vec3d XYname;
+	if (!sPainter.getProjector()->projectCheck(XYZname, XYname))
 	{
 		return;
 	}
 
-	sPainter.getProjector()->project(XYZname, XYname);
 	sPainter.setColor(labelColor, 1.0f);
-	sPainter.drawText(static_cast<float>(XYname[0]), static_cast<float>(XYname[1]), englishName, 0., -sPainter.getFontMetrics().boundingRect(englishName).width()/2, 0, false);
+	sPainter.drawText(static_cast<float>(XYname[0]), static_cast<float>(XYname[1]), englishName, 0.,
+	                  -sPainter.getFontMetrics().boundingRect(englishName).width() / 2, 0, false);
 }
 
-void scm::ScmConstellation::drawNames(StelCore *core, StelPainter sPainter)
+void scm::ScmConstellation::drawNames(StelCore *core, StelPainter &sPainter) const
 {
 	drawNames(core, sPainter, colorLabelDefault);
 }
 
-
-QJsonObject scm::ScmConstellation::toJson(QString &skyCultureName) const
+QJsonObject scm::ScmConstellation::toJson(const QString &skyCultureName) const
 {
 	QJsonObject json;
 
@@ -120,7 +122,7 @@ QJsonObject scm::ScmConstellation::toJson(QString &skyCultureName) const
 	if (constellationStars.size() != 0)
 	{
 		// Stars are NOT empty
-		for (const auto &star: constellationStars)
+		for (const auto &star : constellationStars)
 		{
 			linesArray.append(star.toJson());
 		}
@@ -134,7 +136,7 @@ QJsonObject scm::ScmConstellation::toJson(QString &skyCultureName) const
 		}
 	}
 
-	json["id"] = "CON " + skyCultureName + " " + id;
+	json["id"]    = "CON " + skyCultureName + " " + id;
 	json["lines"] = linesArray;
 
 	// Assemble common name object
@@ -155,7 +157,7 @@ QJsonObject scm::ScmConstellation::toJson(QString &skyCultureName) const
 	if (references.has_value() && !references->isEmpty())
 	{
 		QJsonArray refsArray;
-		for (const auto& ref : references.value())
+		for (const auto &ref : references.value())
 		{
 			refsArray.append(ref);
 		}
@@ -164,4 +166,15 @@ QJsonObject scm::ScmConstellation::toJson(QString &skyCultureName) const
 	json["common_name"] = commonNameObj;
 
 	return json;
-}
+}
+
+void scm::ScmConstellation::updateTextPosition()
+{
+	XYZname.set(0., 0., 0.);
+	for (CoordinateLine p : constellationCoordinates)
+	{
+		XYZname += p.end;
+		XYZname += p.start;
+	}
+	XYZname.normalize();
+}