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.

Sign up for GitHub

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

Jump to bottom

Fix: orbit details #3348

Merged
merged 8 commits into from
Aug 6, 2023
Merged

Fix: orbit details #3348

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
af03fc0
Add another option to allow storing config items immediately
gzotti Aug 1, 2023
c6c3411
Fix the botched logic of storing details for the orbit drawing (Fix #…
gzotti Aug 1, 2023
c22580f
Add static method to StelApp to save immediately (simplifies syntax)
gzotti Aug 3, 2023
d4c0aa1
apply review suggestions
gzotti Aug 3, 2023
7b7498b
Add option to add or exclude moon orbits for planets/selected objects
gzotti Aug 3, 2023
5bbc045
Cleanup
gzotti Aug 4, 2023
8b0088a
SUG: Add new details
gzotti Aug 4, 2023
52e9675
Fixed a logic error introduced by previous edit.
gzotti Aug 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
201 changes: 131 additions & 70 deletions src/core/modules/SolarSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,9 @@
planetNameFont.setPixelSize(StelApp::getInstance().getScreenFontSize());
connect(&StelApp::getInstance(), SIGNAL(screenFontSizeChanged(int)), this, SLOT(setFontSize(int)));
setObjectName("SolarSystem");
connect(this, SIGNAL(flagOrbitsChanged(bool)), this, SLOT(reconfigureOrbits()));
connect(this, SIGNAL(flagPlanetsOrbitsOnlyChanged(bool)), this, SLOT(reconfigureOrbits()));
connect(this, SIGNAL(flagIsolatedOrbitsChanged(bool)), this, SLOT(reconfigureOrbits()));
}

void SolarSystem::setFontSize(int newFontSize)
Expand Down Expand Up @@ -274,8 +277,8 @@
setEphemerisJupiterMarkerColor( Vec3f(conf->value("color/ephemeris_jupiter_marker_color", "0.3,1.0,1.0").toString()));
setEphemerisSaturnMarkerColor( Vec3f(conf->value("color/ephemeris_saturn_marker_color", "0.0,1.0,0.0").toString()));

setOrbitsThickness(conf->value("astro/object_orbits_thickness", 1).toBool());
setTrailsThickness(conf->value("astro/object_trails_thickness", 1).toBool());
setOrbitsThickness(conf->value("astro/object_orbits_thickness", 1).toInt());
setTrailsThickness(conf->value("astro/object_trails_thickness", 1).toInt());
recreateTrails();
setFlagTrails(conf->value("astro/flag_object_trails", false).toBool());

Expand Down Expand Up @@ -2050,67 +2053,6 @@
return false;
}

void SolarSystem::setFlagOrbits(bool b)
{
bool old = flagOrbits;
flagOrbits = b;
bool flagPlanetsOnly = getFlagPlanetsOrbitsOnly();
if (!b || !selected || selected==sun)
{
if (flagPlanetsOnly)
{
for (const auto& p : qAsConst(systemPlanets))
{
if (p->getPlanetType()==Planet::isPlanet)
p->setFlagOrbits(b);
else
p->setFlagOrbits(false);
}
}
else
{
for (const auto& p : qAsConst(systemPlanets))
p->setFlagOrbits(b);
}
}
else if (getFlagIsolatedOrbits()) // If a Planet is selected and orbits are on, fade out non-selected ones
{
if (flagPlanetsOnly)
{
for (const auto& p : qAsConst(systemPlanets))
{
if (selected == p && p->getPlanetType()==Planet::isPlanet)
p->setFlagOrbits(b);
else
p->setFlagOrbits(false);
}
}
else
{
for (const auto& p : qAsConst(systemPlanets))
{
if (selected == p)
p->setFlagOrbits(b);
else
p->setFlagOrbits(false);
}
}
}
else
{
// A planet is selected and orbits are on - draw orbits for the planet and their moons
for (const auto& p : qAsConst(systemPlanets))
{
if (selected == p || selected == p->parent)
p->setFlagOrbits(b);
else
p->setFlagOrbits(false);
}
}
if(old != flagOrbits)
emit flagOrbitsChanged(flagOrbits);
}

void SolarSystem::setFlagLightTravelTime(bool b)
{
if(b!=flagLightTravelTime)
Expand Down Expand Up @@ -2142,7 +2084,8 @@
selected.clear();
// Undraw other objects hints, orbit, trails etc..
setFlagHints(getFlagHints());
setFlagOrbits(getFlagOrbits());
//setFlagOrbits(getFlagOrbits());
reconfigureOrbits();
}


Expand Down Expand Up @@ -2668,14 +2611,120 @@
emit numberIsolatedTrailsChanged(numberIsolatedTrails);
}

void SolarSystem::setFlagOrbits(bool b)
{
if(b!=getFlagOrbits())
{
flagOrbits = b;
emit flagOrbitsChanged(b);
}
}

// Connect this to all signals when orbit selection or selected object has changed.
// This method goes through all planets and sets orbit drawing as configured by several flags
void SolarSystem::reconfigureOrbits()
{
// we have: flagOrbits O, flagIsolatedOrbits I, flagPlanetsOrbitsOnly P, flagPermanentOrbits and a possibly selected planet S
// permanentOrbits only influences local drawing of a single planet and can be ignored here.
// O S I P
// 0 X X X NONE
// 1 0 1 X NONE
// 1 X 0 0 ALL
// 1 X 0 1 all planets only

// 1 1 1 0 only selected planet and orbits of its moon system
// 1 1 1 1 only selected SSO if it is a major planet

if (!flagOrbits || (flagOrbits&&flagIsolatedOrbits&&(!selected || selected==sun)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, same redundancy with flagOrbits here as the ones below.

{
for (const auto& p : qAsConst(systemPlanets))
p->setFlagOrbits(false);
return;
}
// from here, flagOrbits is certainly on
if (!flagIsolatedOrbits)
{
for (const auto& p : qAsConst(systemPlanets))
if (!flagPlanetsOrbitsOnly || (flagPlanetsOrbitsOnly && (p->getPlanetType()==Planet::isPlanet || (p->parent && p->parent->getPlanetType()==Planet::isPlanet) )))
Copy link
Contributor

@10110111 10110111 Aug 2, 2023 •
edited

Choose a reason for hiding this comment

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

!flagPlanetsOrbitsOnly || (flagPlanetsOrbitsOnly &&...) is redundant: the second check for the flag yields true whenever it's evaluated. After you remove it, you can also get rid of two pairs of parentheses, yielding this simpler condition:

if (!flagPlanetsOrbitsOnly || p->getPlanetType()==Planet::isPlanet ||
    (p->parent && p->parent->getPlanetType()==Planet::isPlanet))

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, indeed. This is currently written mostly documentarily.

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern here is that it makes it harder to understand the condition, not only because of an extra check, but also due to multiple nested levels of parentheses.

p->setFlagOrbits(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here about passing a Boolean condition.

else
p->setFlagOrbits(false);
return;
}
else // flagIsolatedOrbits && selected
{
// Display only orbit for selected planet and its moons.
for (const auto& p : qAsConst(systemPlanets))
if ( (p==selected && ( !flagPlanetsOrbitsOnly || (flagPlanetsOrbitsOnly && p->getPlanetType()==Planet::isPlanet ) )
Copy link
Contributor

Choose a reason for hiding this comment

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

Same redundancy of the second check of flagPlanetsOrbitsOnly here.

|| (p->getPlanetType()==Planet::isMoon && p->parent==selected ) ))
p->setFlagOrbits(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're basically passing the result of the condition check, you can change
if(cond) set(true); else set(false); to set(cond);.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I just followed the existing style, but we can change that.

else
p->setFlagOrbits(false);
return;
}

/*
if (!selected || selected==sun)
{
if (flagPlanetsOrbitsOnly)
{
for (const auto& p : qAsConst(systemPlanets))
{
if (p->getPlanetType()==Planet::isPlanet)
p->setFlagOrbits(true);
else
p->setFlagOrbits(false);
}
}
}
else if (getFlagIsolatedOrbits()) // If a Planet is selected and orbits are on, fade out non-selected ones
{
if (flagPlanetsOrbitsOnly)
{
for (const auto& p : qAsConst(systemPlanets))
{
if (selected == p && p->getPlanetType()==Planet::isPlanet)
p->setFlagOrbits(b);
else
p->setFlagOrbits(false);
}
}
else
{
for (const auto& p : qAsConst(systemPlanets))
{
if (selected == p)
p->setFlagOrbits(b);
else
p->setFlagOrbits(false);
}
}
}
else
{
// A planet is selected and orbits are on - draw orbits for the planet and their moons
for (const auto& p : qAsConst(systemPlanets))
{
if (selected == p || selected == p->parent)
p->setFlagOrbits(b);
else
p->setFlagOrbits(false);
}
}
*/
}

Check notice on line 2716 in src/core/modules/SolarSystem.cpp

View check run for this annotation

codefactor.io / CodeFactor

src/core/modules/SolarSystem.cpp#L2625-L2716 

Complex Method
void SolarSystem::setFlagIsolatedOrbits(bool b)
{
if(b!=flagIsolatedOrbits)
{
flagIsolatedOrbits = b;
if (StelApp::getInstance().getFlagImmediateSave())
conf->setValue("viewing/flag_isolated_orbits", b);

emit flagIsolatedOrbitsChanged(b);
// Reinstall flag for orbits to renew visibility of orbits
setFlagOrbits(getFlagOrbits());
//setFlagOrbits(getFlagOrbits());
}
}

Expand All @@ -2689,9 +2738,11 @@
if(b!=flagPlanetsOrbitsOnly)
{
flagPlanetsOrbitsOnly = b;
if (StelApp::getInstance().getFlagImmediateSave())
conf->setValue("viewing/flag_planets_orbits_only", b);
emit flagPlanetsOrbitsOnlyChanged(b);
// Reinstall flag for orbits to renew visibility of orbits
setFlagOrbits(getFlagOrbits());
//setFlagOrbits(getFlagOrbits());
}
}

Expand Down Expand Up @@ -3276,8 +3327,13 @@

void SolarSystem::setFlagPermanentOrbits(bool b)
{
Planet::permanentDrawingOrbits=b;
emit flagPermanentOrbitsChanged(b);
if (Planet::permanentDrawingOrbits!=b)
{
Planet::permanentDrawingOrbits=b;
if (StelApp::getInstance().getFlagImmediateSave())
conf->setValue("astro/flag_permanent_orbits", b);
emit flagPermanentOrbitsChanged(b);
}
}

bool SolarSystem::getFlagPermanentOrbits() const
Expand All @@ -3287,8 +3343,13 @@

void SolarSystem::setOrbitsThickness(int v)
{
Planet::orbitsThickness=v;
emit orbitsThicknessChanged(v);
if (v!=Planet::orbitsThickness)
{
Planet::orbitsThickness=v;
if (StelApp::getInstance().getFlagImmediateSave())
conf->setValue("astro/object_orbits_thickness", v);
emit orbitsThicknessChanged(v);
}
}

int SolarSystem::getOrbitsThickness() const
Expand Down
9 changes: 4 additions & 5 deletions src/core/modules/SolarSystem.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,19 +53,16 @@ class SolarSystem : public StelObjectModule
Q_OBJECT
// This is a "forwarding property" which sets labeling into all planets.
Q_PROPERTY(bool labelsDisplayed READ getFlagLabels WRITE setFlagLabels NOTIFY labelsDisplayedChanged)
// was bool orbitsDisplayed
Q_PROPERTY(bool flagOrbits READ getFlagOrbits WRITE setFlagOrbits NOTIFY flagOrbitsChanged)
Q_PROPERTY(bool trailsDisplayed READ getFlagTrails WRITE setFlagTrails NOTIFY trailsDisplayedChanged)
Q_PROPERTY(int maxTrailPoints READ getMaxTrailPoints WRITE setMaxTrailPoints NOTIFY maxTrailPointsChanged)
Q_PROPERTY(int maxTrailTimeExtent READ getMaxTrailTimeExtent WRITE setMaxTrailTimeExtent NOTIFY maxTrailTimeExtentChanged)
Q_PROPERTY(int trailsThickness READ getTrailsThickness WRITE setTrailsThickness NOTIFY trailsThicknessChanged)
// was bool hintsDisplayed. This is a "forwarding property" only, without own variable.
// This is a "forwarding property" only, without own variable.
Q_PROPERTY(bool flagHints READ getFlagHints WRITE setFlagHints NOTIFY flagHintsChanged)
// was bool pointersDisplayed
Q_PROPERTY(bool flagPointer READ getFlagPointer WRITE setFlagPointer NOTIFY flagPointerChanged)
// was bool nativeNamesDisplayed
Q_PROPERTY(bool flagNativePlanetNames READ getFlagNativePlanetNames WRITE setFlagNativePlanetNames NOTIFY flagNativePlanetNamesChanged)
Q_PROPERTY(bool planetsDisplayed READ getFlagPlanets WRITE setFlagPlanets NOTIFY flagPlanetsDisplayedChanged)
Q_PROPERTY(bool flagOrbits READ getFlagOrbits WRITE setFlagOrbits NOTIFY flagOrbitsChanged)
Q_PROPERTY(bool flagPlanetsOrbitsOnly READ getFlagPlanetsOrbitsOnly WRITE setFlagPlanetsOrbitsOnly NOTIFY flagPlanetsOrbitsOnlyChanged)
Q_PROPERTY(bool flagPermanentOrbits READ getFlagPermanentOrbits WRITE setFlagPermanentOrbits NOTIFY flagPermanentOrbitsChanged)
Q_PROPERTY(bool flagIsolatedOrbits READ getFlagIsolatedOrbits WRITE setFlagIsolatedOrbits NOTIFY flagIsolatedOrbitsChanged)
Expand Down Expand Up @@ -998,6 +995,8 @@ private slots:
//! Taking the JD dates for each ephemeride and preparation the human readable dates according to the settings for dates
void fillEphemerisDates();

//! When some aspect of orbit drawing changes, update their configuration
void reconfigureOrbits();
private:
//! Search for SolarSystem objects which are close to the position given
//! in earth equatorial position.
Expand Down
9 changes: 6 additions & 3 deletions src/gui/viewDialog.ui
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1685,6 +1685,9 @@
</property>
<item>
<widget class="QCheckBox" name="planetOrbitOnlyCheckBox">
<property name="toolTip">
<string>Hide orbits of minor bodies.</string>
</property>
<property name="text">
<string>Only orbits of major planets</string>
</property>
Expand All @@ -1693,7 +1696,7 @@
<item>
<widget class="QCheckBox" name="planetIsolatedOrbitCheckBox">
<property name="toolTip">
<string>Deactivate this option if you want to see orbit for selected planet and its moons.</string>
<string>Activate this option if you want to see orbit for selected planet and its moons.</string>
</property>
<property name="text">
<string>Only orbit for selected object</string>
Expand Down Expand Up @@ -5690,12 +5693,12 @@
</resources>
<connections/>
<buttongroups>
<buttongroup name="buttonGroupDisplayedDSOCatalogs">
<buttongroup name="buttonGroupDisplayedDSOTypes">
<property name="exclusive">
<bool>false</bool>
</property>
</buttongroup>
<buttongroup name="buttonGroupDisplayedDSOTypes">
<buttongroup name="buttonGroupDisplayedDSOCatalogs">
<property name="exclusive">
<bool>false</bool>
</property>
Expand Down