Improve automatic star selection in the sector map

[Gliese852]

During manual movement in the sector map, when the option is enabled, the closest star to the map viewing position is automatically selected. But the search is performed only in the sector containing the original position, so the wrong star may be highlighted. This commit fixes this defect, now the search is performed in an increasing (if necessary) range, and the really closest star is found.

Also, fixed that the star was not automatically selected when animating "by inertia", but only when a key was pressed.

[Web-eWorks]

At great length (and with no shortage of internal deliberation), I'm going to have to ask that this code be revised before it's merged.

Though it's certainly a valid way to solve the problem, the inline type declarations of an ad-hoc integer AABB type (and the associated functions) make the code difficult to read, and the expanding-search implementation is far more complicated than necessary for the limited scope of its usage.

I would like to see an implementation that uses the existing AABB type to search at most the 3x3 cube of sectors surrounding the current sector, returning no selection if there are in fact no systems there. A lot of the logic I'm reading in the current implementation - to me, at least - seems to ignore the principle of YAGNI: "you aren't going to need it". I'm laying out a few points as review comments that would need to be changed whether or not this implementation was approved, with the intent of advice and not attacking your code directly.

I know this is a bit hypocritical of me to deny merging code that I myself asked you to write, but it is my firmly-held opinion that merging this code as-is would not have a positive effect on the codebase - it doesn't follow the idioms or types of the surrounding code, and it took me a good five minutes to begin to understand the control flow and especially the math in the hand-rolled AABB type.

That was a whole lot of negatives without many positives, so for what I do like about this code:

• It selects systems by closest distance, the most critical part of the PR!
• It's extremely well commented, to the point where I almost don't need to read the actual C++ to understand the ins and outs of the function. (Although requiring comments to know what's going on in a function is considered by some to be a code smell, I personally appreciate comments that explain both the what and the why of a function's implementation.)
• Functions and variables are well-named and make sense when read in English.

[Web-eWorks]

It occurs to me (perhaps a bit late) that I may not have clearly communicated what I originally envisioned the selection behavior functioning as. I'd envisioned the selection functioning with a maximum radius that does a single-pass search over sectors within that radius (perhaps max.x = center.x + ceil(radius / SECTOR_SIZE)), either returning the closest system within that radius or returning no selection if the player has found a dense enough region of space.

You've certainly gone above and beyond that requested behavior, and I'm of two minds as to whether an expanding selection radius until a system is found is a good idea - my thought is that it could potentially select systems offscreen if density in the viewed region of space is low enough. The current behavior is certainly acceptable, although it would require some cleanup and clarification of control flow before I'm comfortable merging it.

[Gliese852]

@Web-eWorks

the inline type declarations of an ad-hoc integer AABB type

But it's not a type (from the certain point of view©), it's just an array, I can remove using

I would like to see an implementation that uses the existing AABB

I didn't use this structure for several reasons:

1. It contains real numbers, but I need integers (mostly), and I am suspicious of checking real numbers for equality.
2. I have not seen examples of using AABB with sector coordinates in the code, it seems that this is meant to store the dimensions of a certain model.
3. AFAIK I cannot iterate over all the elements of this structure as an array (without modifications), and instead of one loop on all sides of the box, I have to write 6 approximately identical pieces of code (I mean, I can't think of any other way to achieve this yet)

to search at most the 3x3 cube of sectors surrounding the current sector

If you just blindly do this, you have to check 27 sectors, and discard all results that are further than 1 sector (8 ly), Otherwise, you cannot guarantee that you did not miss a closer point.

About searching in a sphere of fixed radius, this approach seems to be quite good, if first select all sectors that exactly fall into this sphere, and then discard the results that do not fall into this sphere. An important question - what is this radius, and how many sectors we are must to scan, to find that the nearest star is 1 light year away from the cursor pos?

My algorithm will scan 1 or 2 sectors in most cases (8 in very most cases). But I really don't know how important it is.

ADD:
About selecting the offscreen star - now the maximum size of the search box is 20 sectors, const int MAX_SEARCH_DIST = 20; this is a configurable parameter. Perhaps we can even change it, depending on how many sectors are visible on the screen.

ADD:
Though I overestimated it a little, saying "1-2 sectors", you can see here how this works:

secsearch.mp4

[Web-eWorks]

AFAIK I cannot iterate over all the elements of this structure as an array (without modifications), and instead of one loop on all sides of the box, I have to write 6 approximately identical pieces of code (I mean, I can't think of any other way to achieve this yet)

I withdraw the request to use the existing AABB now that I see how you're using this capability for optimization purposes, especially the distance-to-side calculation. I'd emphasize the need for comments that clarify the nature of that algorithm, because otherwise a lot of the written code seems to be overengineered and needlessly complicated.

If you just blindly do this, you have to check 27 sectors, and discard all results that are further than 1 sector (8 ly), Otherwise, you cannot guarantee that you did not miss a closer point.

If the sector the "focal point" resides in has no systems (a fairly common occurrence around Sol, I counted at least 4-5 empty sectors within 6-8 sectors and your video pointed out it happens a lot more than even I thought), you're already skipping straight to searching 27 sectors in the cube around the focal point. I don't think that this search, performed only one or a few times per frame is going to cause a particular performance issue by iterating through ~64 systems per test. Indeed, this provides a better upper-bound in low density regions of space, and also resolves the case where the closest system may well be behind the camera instead of the visible but slightly further distant system the user expects it to select.

About searching in a sphere of fixed radius, this approach seems to be quite good, if first select all sectors that exactly fall into this sphere, and then discard the results that do not fall into this sphere. An important question - what is this radius, and how many sectors we are must to scan, to find that the nearest star is 1 light year away from the cursor pos?

My suggestion was to merely search an 8ly radius (1 sector width) around the cursor, clearing the selection if nothing was found. In this way, only a 3x3 cube needs be searched, even if the focal point lies near or on the edge of the current sector. However, if this search function is meant to be used as a more general "find system closest to point" for more than just selecting systems based on the user's camera position, I see why it is implemented as such, though I'd request that it be more well-integrated with the SectorMap API.

About selecting the offscreen star - now the maximum size of the search box is 20 sectors, const int MAX_SEARCH_DIST = 20; this is a configurable parameter. Perhaps we can even change it, depending on how many sectors are visible on the screen.

If this is the maximum "diameter" of the search box (rather than the half-extent of any given axis), I'd recommend dropping it to 12-16 instead of 20 - 9100 sectors is a little steep as an upper-bound worst case.

But it's not a type (from the certain point of view©), it's just an array, I can remove using

I will leave a review comment about the problematic usage of using - this one was more an issue that the std::array<int, SIDES> (with or without the using statement) was a red-flag to my sense of "code smell", it indicates that the following code tried to re-invent the wheel rather than doing something sensible but boring.

It's a bit of a humorous idiom among software engineers, but if a piece of code seems 'clever' when you're writing it, that's usually a big red flag that it will wind up incomprehensible and a pain to maintain by others (to say nothing of yourself). Doesn't always work out that way, but anytime you think you're writing smart or clever code it's like a red sky in the morning - coders should take warning.

In this case, making the code comprehensible again is mostly a matter of fixing the comments to no longer be misleading and addressing a few issues I'll leave in review comments,

[Web-eWorks]

Thank you @Gliese852 for all of your hard work on this PR and for putting up with my detailed and sometimes pedantic reviews! I'll try to cut a release a little later today once I've built everything locally and it all works!