Fix issue #1842

[A-j-K]

Description

The change is to invert the logic calling setFlagOrbits() which seemed to be the wrong way around.

Fixes #1842

Screenshots (if appropriate):

n/a

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?

Tested on Windows 10 Visual Studio before and after the change.

Test Configuration:

• Operating system: Windows 10
• Graphics Card: n/a

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
• 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?

This is an automatically generated QA checklist based on modified files

[gzotti]

Can you describe in which way you feel the logic is "the wrong way around"?

[A-j-K]

Can you describe in which way you feel the logic is "the wrong way around"?

The action at this point is going from "orbit display object only" to "orbit display all objects". So for all objects to show the orbit setFlagOrbits() should be set true (switch it on). But false was here meaning "switch them off" which isn't the desired behaviour.

[gzotti]

Ah, indeed there is something odd here. But now I can select Venus and click "only orbit for major planets", and see all orbits. When I deselect Venus the other orbits vanish as intended by the checkboxes.
The logic of this method is indeed not simple to get completely right. Maybe you can work out all cases?

I am not completely sure when to display the orbit system of planet moons. When I select Jupiter and "show orbit for selected object only", should its moon system "blossom up" as well? Or, when I select Io, show really just Io's orbit or those of all other moons? Or single moon orbit plus parent orbit? There are lots of possibilities here. @alex-w ?

[A-j-K]

Ah, indeed there is something odd here. But now I can select Venus and click "only orbit for major planets", and see all orbits. When I deselect Venus the other orbits vanish as intended by the checkboxes. The logic of this method is indeed not simple to get completely right. Maybe you can work out all cases?

I am not completely sure when to display the orbit system of planet moons. When I select Jupiter and "show orbit for selected object only", should its moon system "blossom up" as well? Or, when I select Io, show really just Io's orbit or those of all other moons? Or single moon orbit plus parent orbit? There are lots of possibilities here. @alex-w ?

Indeed it was something of a struggle that I initially expected to be straightforward. There are just 2 or 3 checkboxes in play (4 or 8 outcomes). However, the current state is also an input and there are variations (for example moons) that complicate it. @gzotti if it's ok with you I think I'd like to replace the complex set of nested "if/else" structures with a strategy pattern that should be simpler to follow (as a human). If you agree that's the way forward I'd be happy to look at that and we can suspend this PR for the time being?

[gzotti]

Yes, please give this a try and if you want rewrite the method, as long as the result does what it should do, and the code becomes at least not worse. I usually write comments while thinking through the logic, before I fill in the actual code. No need to suspend the PR, just continue on this branch and keep pushing. When it works as we all find perfect, we merge the branch.

[A-j-K]

Yes, please give this a try and if you want rewrite the method, as long as the result does what it should do, and the code becomes at least not worse. I usually write comments while thinking through the logic, before I fill in the actual code. No need to suspend the PR, just continue on this branch and keep pushing. When it works as we all find perfect, we merge the branch.

I was just reading the guide to try and determine what the expected behaviour should be and I see that the checkboxes here are not described in the guide (but there are references to the flags in the configuration appendix). So, I will work based on what I understand today but this may well lead to some questions here to clarify the exact expected behaviour so we can reach that goal. I'll start on it later this evening, I'm at my day job at the moment ;)

[gzotti]

The button tooltips should explain the intentions. Not every minuscule detail is therefore explained in the Guide, but thanks for looking up :-). Sure, take your time, the next release is >4 weeks in the future.

[A-j-K]

Yeah, I'm running into confusion understanding the requirements. I tried to "build my own" based on how I think it should work and pretty much failed. Here's why:-

I am not completely sure when to display the orbit system of planet moons. When I select Jupiter and "show orbit for selected object only", should its moon system "blossom up" as well? Or, when I select Io, show really just Io's orbit or those of all other moons? Or single moon orbit plus parent orbit? There are lots of possibilities here. @alex-w ?

There is essentially nothing in the checkbox descriptions, tooltips, or Stellarium Guide about moons of major planets. So figuring out when a moon's orbit should be shown is not obvious.

We have three checkboxes:-

• Show orbits (a)
• Only orbits of major planets (b)
• Only orbit for selected object (c)

Now "Show orbits" (a) just switches them all on or off. That leaves two checkboxes where we can just draw up a basic truth table:-

  (b)    (c)
   0      0    Show all orbits
   0      1    Only show the orbit of the selected object (no moons if major planet selected)
   1      0    Only show orbits of major planets (but what about moons?)
   1      1    Only show orbits of selected major planets (again, what about moons?)

So, at this point, I can implement the above truth table (where moons can be seen if selected but not its parent or siblings).

But I actually think I need a lead here from @alex-w or @gzotti on the requirements and expectations. I could make it up myself but I have learnt that making assumptions about other peoples expectations is not the best way forward.

My gut feeling is that an extra checkbox is needed to describe the expected behaviour of the moons etc. But that's starting to get a bit much (3 checkboxes is eight combinations).

Thoughts/comments?

[gzotti]

The point is the difference between selection and the checkboxes, so there are indeed 8 cases (apart from the Orbits general switch)

(b) Only orbits of major planets
(c) Only orbit for selected object
(s) Object has been selected

My personal understanding when "limited" to the current 2 detailing checkboxes and "selected" item

(b)  (c)  (s)
0    0  nc    Show all orbits
0    1   0     Show nothing
0    1   1     Only show the orbit of the selected object (**with** moons if a major planet selected)
1    0   0     Only show orbits of major planets 
1    0   1     Only show orbits of major planets plus the moons of a potentially selected major planet, 
                 or the orbits of major planets plus the orbit of a selected minor body
                 or the orbits of major planets plus the orbit of one selected moon around its planet
1    1   0     Show nothing OR (TBD) Only show orbits of major planets 
1    1   1     Only show orbit of selected object plus orbits of its moons.

The case 110 implies showing nothing, but maybe it is better to have the major planet orbits.
@alex-w any opinion?

[alex-w]

Sorry for late answer. When I've added new options to this feature (orbit rendering) then I tried saving the strong backward compatibility to behaviour of orbits. Probably the time to changes the behaviour of orbits and logic of the tool is coming. So, I think existed feature and requests should be revised and, maybe, reimplemented or just documented (in the SUG and GUI) current behaviour with fixes.

@gzotti @A-j-K opinions or preferences?

[gzotti]

I think we have seen the current logic is incomplete or even buggy. I have described above what I would see as most comprehensive and useful. Of course YMMV, so we can discuss other interpretations/drafts.

[github-actions]

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

[gzotti]

This has finally been superseded and solved by #3348. Thanks for discussing!