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

merged 8 commits into from Aug 6, 2023
+158 −84

Conversation

gzotti
Copy link
Member

@gzotti gzotti commented Aug 2, 2023 •
edited

Description

As discussed in #2043, the sequence of selection around orbit details is inconsistent and buggy.
I separated flag setting from the actual selection (which involves 4 flags and an optional selection).
We can discuss yet another finetuning option "with moons" which would influence whether the orbital system of moons would be highlighted even if the flag for only "major planets" or "only selected object" is set and the selected object is a major planet.

Also, as first commit, I introduce a (currently hidden and manually editable only) option StelApp.flagImmediateSave. If set, this can be used to store settings immediately. The intention is to allow storing detail settings in the GUI, like orbit drawing details, DSO catalog selection or type filtering, without having to save these globally (and thereby maybe accidentally overwrite and globally store other settings that were active only during this session). My personal preference would be to get rid of the global save entirely, but I see resistance. Maybe we can offer a global switch on "immediate save" vs "global save", when all settings have been adjusted over the next few months.

I will amend the second commit by removing commented code and optionally adding the "with moons" option.

After this has been merged, PR #3342 will be adjusted accordingly.

Fixes #1842 (supersedes PR #2043)

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: Windows 11
  • Graphics Card: Geforce 3070Ti (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
Add another option to allow storing config items immediately
af03fc0 
- the intention is to start using this for storing detail settings
without having to do the global store.
@gzotti gzotti added bug Something likely wrong in the code enhancement Improve existing functionality importance: medium A bit annoying, minor miscalculation, but no crash labels Aug 2, 2023
@gzotti gzotti added this to the 23.3 milestone Aug 2, 2023
@gzotti gzotti self-assigned this Aug 2, 2023
@gzotti gzotti added this to Needs triage in GUI via automation Aug 2, 2023
@gzotti gzotti added this to Needs triage in Visualization via automation Aug 2, 2023
@github-actions
Copy link

github-actions bot commented Aug 2, 2023

Great PR! Please pay attention to the following items before merging:

Files matching src/**/*.cpp:

  • Are possibly unused includes removed?

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

@gzotti
Fix the botched logic of storing details for the orbit drawing (Fix #…
c6c3411 
…1842)

- This has been attempted also in #2043, but I think this is more comprehensive.
- In addition, immediate-mode storage has been added to the Orbit details.
- In addition, a bug about storing line width has been fixed.
alex-w
alex-w requested changes Aug 2, 2023
View reviewed changes
src/core/modules/SolarSystem.cpp Outdated Show resolved Hide resolved
Visualization automation moved this from Needs triage to In progress Aug 2, 2023
GUI automation moved this from Needs triage to In progress Aug 2, 2023
10110111
10110111 reviewed Aug 2, 2023
View reviewed changes
src/core/modules/SolarSystem.cpp Outdated
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.

10110111
10110111 reviewed Aug 2, 2023
View reviewed changes
src/core/modules/SolarSystem.cpp Outdated
{
// 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.

10110111
10110111 reviewed Aug 2, 2023
View reviewed changes
src/core/modules/SolarSystem.cpp Outdated
for (const auto& p : qAsConst(systemPlanets))
if ( (p==selected && ( !flagPlanetsOrbitsOnly || (flagPlanetsOrbitsOnly && p->getPlanetType()==Planet::isPlanet ) )
|| (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.

10110111
10110111 reviewed Aug 2, 2023
View reviewed changes
src/core/modules/SolarSystem.cpp Outdated
// 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.

10110111
10110111 reviewed Aug 2, 2023
View reviewed changes
src/core/modules/SolarSystem.cpp Outdated
{
for (const auto& p : qAsConst(systemPlanets))
if (!flagPlanetsOrbitsOnly || (flagPlanetsOrbitsOnly && (p->getPlanetType()==Planet::isPlanet || (p->parent && p->parent->getPlanetType()==Planet::isPlanet) )))
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.

@gzotti
Copy link
Member Author

gzotti commented Aug 2, 2023

What about functionality? Shall moons for the planets be co-selected when "planet only" is active? Shall the single selected planet also show its moons' orbits? Or should we deal with the moons with yet another flag? Shall a selected object like the Moon (or other planet moons) show its orbit even if "only planet orbits" and "selected only" are active?

@Atque
Copy link
Contributor

Atque commented Aug 2, 2023

What about a whole new setting planet + moon orbit?

@gzotti
Copy link
Member Author

gzotti commented Aug 2, 2023

This is what I am asking: Show moons automatically (like now), or only if allowed by another switch?

@Atque
Copy link
Contributor

Atque commented Aug 2, 2023

This is what I am asking: Show moons automatically (like now), or only if allowed by another switch?

Oh yeah, sorry. A new flag, I would prefer.

@gzotti
Copy link
Member Author

gzotti commented Aug 3, 2023

@alex-w , @10110111 your opinions/wishes/comments on that?

@10110111
Copy link
Contributor

10110111 commented Aug 3, 2023

My wish is that what's written on the checkbox, that would be seen in the scene, without surprises. If the checkbox says only selected planet, then only the planet's orbit would be visible. So I guess this would mean a radiobutton:

ⵙ selected planet
ⵔ selected planet and its moons

@alex-w
Copy link
Member

alex-w commented Aug 3, 2023

I don’t like “immediate save” implementation - I think it should be implemented as new class/wrapper with static methods for saving the options + public method for saving all options.

@gzotti
Copy link
Member Author

gzotti commented Aug 3, 2023

I dislike the StelApp::getInstance() ... verbosity.
You mean something shorter like a static StelApp::immediateStore("option", value) which queries the flag and only stores if StelApp.flagImmediateSave has been activated?

Note that for now all these these settings are also still stored in ConfigDialog, so if immediate saving is not active, behaviour is unchanged.

@gzotti
Copy link
Member Author

gzotti commented Aug 5, 2023

@alex-w , @10110111 any more comments or objections?

10110111
10110111 approved these changes Aug 5, 2023
View reviewed changes
Copy link
Contributor

@10110111 10110111 left a comment

Choose a reason for hiding this comment

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

Seems to work as expected.

@gzotti gzotti merged commit dd6ca5b into master Aug 6, 2023
20 checks passed
GUI automation moved this from In progress to Done Aug 6, 2023
@gzotti gzotti deleted the fix/orbit_details branch August 6, 2023 08:23
Visualization automation moved this from In progress to Done Aug 6, 2023
@gzotti gzotti mentioned this pull request Aug 6, 2023
Closed
12 tasks
@alex-w alex-w added the state: published The fix has been published for testing in weekly binary package label Sep 2, 2023
@github-actions
Copy link

github-actions bot commented Sep 2, 2023

Hello @gzotti!

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

@alex-w alex-w removed the state: published The fix has been published for testing in weekly binary package label Sep 26, 2023
@github-actions
Copy link

Hello @gzotti!

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alex-w alex-w alex-w approved these changes

@10110111 10110111 10110111 approved these changes

Assignees

@gzotti gzotti

Labels
bug Something likely wrong in the code enhancement Improve existing functionality importance: medium A bit annoying, minor miscalculation, but no crash
Projects
GUI
  
Done
Visualization
  
Done
Milestone
23.3
Development

Successfully merging this pull request may close these issues.

Unchecking "Only orbit for selected object" still hides other planets' orbits
4 participants
@gzotti @Atque @10110111 @alex-w