Feat/radar improvements #5984

mwerle · 2024-11-25T05:00:21Z

NOTE: The code could probably do with some tidying up...

ensure targeted object remains visible when switching from automatic to manual zoom (feat(radar): capture mouse wheel for zooming radar #5978 (review))
add intermediate zoom steps for manual zoom (feat(radar): capture mouse wheel for zooming radar #5978 (review))
fix popup disappearing when mouse moves outside of radar area but still over popup
improve checking when mouse is over radar area (especially for the 3D scanner mode display)
add left-click to set navigation target when clicking on a target "pip". This will show a popup if there are multiple targets on a "pip".
add double-click to clear the navigation target

Additionally fix an issue introduced in 678d46e where a couple of parameters are swapped when calling "PlayMusic" (found due to compiler warning).

TODO:

code review
clean up commits
merge

Commit 678d46e added crossfading, but accidentally swapped two parameters when invoking "PlayMusic()".

Ensure that a targeted object will always remain visible on the radar when switching from automatic to manual zoom.

Instead of increasing/decreasing the manual zoom level by powers of 10, use a semi-logarithmic scale of 1,2,5,10... when manually zooming in or out.

data/pigui/modules/radar.lua

mwerle · 2024-11-26T00:15:04Z

data/pigui/modules/radar.lua

+-- Generate the next in a sequence of 1 2 5 10 20 ...
+local function nextZoomSeq(currZoom, maxZoom)
+	local newZoom = 0
+	if getDigits(currZoom) == 2 then
+		newZoom = currZoom * 2.5
+	else
+		newZoom = currZoom * 2
+	end
+	return math.min(newZoom, maxZoom)
+end
+
+-- Generate the previous in a sequence of 1 2 5 10 20 ...
+local function prevZoomSeq(currZoom, minZoom)
+	local newZoom = 0
+	if getDigits(currZoom) == 5 then
+		newZoom = currZoom / 2.5
+	else
+		newZoom = currZoom / 2
+	end
+	return math.max(newZoom, minZoom)
+end
+
+-- Normalize a number into the zoom sequence (1, 2, 5, 10, 20, ...), rounding
+-- it up or down to the next item in the sequence (default: down).
+local function normalizeToZoomSeq(number, up)
+	if up == nil then
+		up = false
+	end
+
+	local firstDigits = getDigits(number, 1, 2)
+	if firstDigits > 50 then
+		firstDigits = up and 10 or 5
+	elseif firstDigits == 50 then
+		firstDigits = 5
+	elseif firstDigits > 20 then
+		firstDigits = up and 5 or 2
+	elseif firstDigits == 20 then
+		firstDigits = 2
+	elseif firstDigits > 10 then
+		firstDigits = up and 2 or 1
+	else
+		firstDigits = 1
+	end
+
+	local numDigits = #tostring(number)
+	return firstDigits * 10^(numDigits-1)
+end
+


These functions are possibly of general use and could be placed into a utility library?

Note that the normalizeToZoomSeq function can be generalized by taking the log10 of the zoom and snapping the fractional part to one of 0, ~0.3, or ~0.7 (for 1, 2, 5 respectively). The particular values should likely be precomputed as math.log10(2) etc. as they have a significant number of digits.

I'll leave the particulars of the implementation to you, but that's how I'd approach the problem.

I'll have a look, although I daresay floating-point arithmetic is going to result in rather imprecise results. I guess performance is moot since it's not going to be calculated every frame..

That said, the core question here was whether these functions should be part of a utility library, and if so where, instead of being in "radar.lua"..

I generally think they should stay in Radar. We can move them out to a utility library if needed, but I don't see a concrete usecase for them anywhere else at this time.

My general philosophy is we only promote things to a utility library once we've identified one or more other areas of the code which have a current or closely-planned future need for that functionality.

I'll leave the particulars of the implementation to you, but that's how I'd approach the problem.

I had a (brief) look, and I'm afraid I really have no idea how to use logs to generate this sequence. I'm not particularly proud of my rather brute-force approach, but it works, was very quick to implement, and everybody can trivially understand it by reading the code.

FWIW, I tried asking ChatGPT (after the IRC discussion yesterday) and it kept providing the same broken solution no matter how much I tried to tell it it was wrong. It eventually realised it was wrong, but couldn't come up with anything else either.
Pattern prediction systems are not "AI", nor do they have any "understanding" of a problem space. They "simply" extend based on previous patterns. Cute toys, and somewhat useful in teasing out patterns out of large data sets, but definitely not something I would consider using for real work. At least not without very carefully reviewing whatever it spits out. As expected, really.

Addendum: just had a look at installing copilot into VSCode, and I am PISSED. They use open-source public repo's for training their models, but don't offer a free version for open-source contributors? I wonder if we can/should amend our open source licenses to include restrictions for tools based off language models trained on the sources..

I've pushed a commit that implements this - feel free to amend or leave as is.

I definitely agree regarding Copilot, it's something I'm not happy with at all.

Thanks. It works and is more concise, but, at least for me, it's not intuitive or understandable. I guess I should re-learn logs sometime.

I will say, I don't consider myself as someone who has a strong background in math (or any background, for that matter) - I wish I could eloquently explain how the above code does what it does, but I only have an intuitive grasp and not an expository grasp of the math involved. Here's an attempt at a long-form explanation:

Fundamentally, the code works via the definition of a logarithm: if $x = b^y$ then the equation can be rewritten as $y = log_b x$.

Since our "primary" zoom steps (1, 10, 100, 1000, etc.) are powers of ten (i.e. $10^0, 10^1, 10^2$), taking the base-10 logarithm of them gives us an integer value corresponding to the order of magnitude; for example, $log_{10}10^3 = 3.0$.

Happily, this means that taking the base-10 logarithm of all numbers between 10,000 and 100,000 gives a decimal result in the range [3..4). Similarly, 100,000 to 1,000,000 gives a decimal result in the range [4..5), and so on.

Each secondary zoom step is always the same regardless of primary zoom step; the only difference is how many zeros follow them (2, 5, 20, 50, etc.). This means that when taking the base-10 logarithm of each secondary zoom step, it will always have the same fractional part regardless of the primary zoom step; only the integer part will change based on which primary zoom step we're in.

The rest of the math is simple at this point - we separate the fractional and integer parts of the logarithm, pick the right fractional part which gives us the proper zoom snap, recombine, and raise 10 to the power of our new exponent.

I've found this resource to be an extremely helpful source for understanding exponents/logarithms - it's quite comprehensive and not dumbed-down like most "Let's Learn Math"-style sites.

Note that all of the above math I outlined can be used in any base, not just base-10 - base-10 just so happens to be the most common for display-to-human applications.

More concisely, this is exploiting the property of logarithms that $log_b(xy) = log_bx + log_by$. Since we functionally have three zoom steps (1, 2, 5) which are multiplied by a power-of-ten distance, we can separate those two components by taking the base-ten logarithm of the final zoom number.

data/pigui/modules/radar.lua

sturnclaw

add double-click to clear the navigation target

General thought - it might be better to have a "reject/unlock target" keybinding which unsets combat then navigation targets in that order. Nav/combat target is generally a state which is managed (by the player) at a higher level than the radar widget.

sturnclaw · 2024-11-26T03:24:07Z

data/pigui/modules/radar.lua

-local click_on_radar = false
+local function onTargetClicked(target)
+	-- TODO: Should ships be set as nav or combat targets?
+	Game.player:SetNavTarget(target.body)


Ships are always combat targets.

Ships are always combat targets.

Not really - rescue missions, for example.

Good point - in general, if you're picking a ship from radar it should be a combat target.

I'd rather add a side-flow to toggle targets between nav/combat states (and possibly keep a "queue" of nav targets... thoughts to explore later) as I anticipate picking a target from radar is likely going to correspond with a player's intent to combat that target.

Fixed in 0665cc5

Does the ship you've targeted as a "Combat Target" instead of as a "Nav Target" "know" how you targeted it?

Ie, does it get an alert "weapons lock" and go into combat-mode itself? Or is this purely a distinction in the player ships' HUD/systems?

Not yet. This is something I want to dive into more deeply in a sensors rework - adding different types of passive information your own sensors can pick up (radar emissions, transponder signals, ship drive plumes, IR/EM flares from missile launches, etc), and giving NPC ships an understanding of them.

With that rework, nav targets would likely be something your computer maintains passively from sensor inputs, with a separate class of targets that your active sensors are maintaining a "trackfile" on, and finally what we currently call the combat target being the primary trackfile on which your sensors are providing good position, velocity, and aspect estimations for firing solutions.

I would also like it to tie into a rework of missiles and countermeasures, introducing different guidance methods and corresponding countermeasure methods to attempt to defeat missile guidance and enemy sensors.

...but that's a topic for another day. Feel free to add this to the dev docs (with your own thoughts on the matter) if you so desire.

data/pigui/modules/radar.lua

sturnclaw · 2024-11-26T03:33:10Z

src/sound/SoundMusic.cpp

@@ -50,7 +50,7 @@ namespace Sound {
 			current = &m_eventTwo;
 			next = &m_eventOne;
 		}
-		next->PlayMusic(name.c_str(), m_volume, repeat, fadeDelta, current);
+		next->PlayMusic(name.c_str(), m_volume, fadeDelta, repeat, current);


Good catch, thank you!

Good catch, thank you!

It helps to compile with BOTH clang and gcc... :)

We -should- set "-Wall, -Weverything, -Werror", at least for release builds, except the "contrib" folder.

No excuse to have a warning in our code. If the code can really not be modified to fix a warning (compilers aren't infallible either), can always set a #pragma with documentation explaining why.

mwerle · 2024-11-26T10:04:15Z

add double-click to clear the navigation target

General thought - it might be better to have a "reject/unlock target" keybinding which unsets combat then navigation targets in that order. Nav/combat target is generally a state which is managed (by the player) at a higher level than the radar widget.

Sure - I just thought it poor that being able to click on a target to set nav-target (can use RMB to set combat target now that we're getting rid of the RMB popup) there was no way to undo that.

Pressing "Y" already resets the nav target, not sure about combat..

EDIT: Some additional testing - if both Combat and Nav targets are set, 'Y' will disable the Combat target first, and then the Nav target. So this already (mostly) works as you suggest - but if there is a target over the reticule, it is set as the target instead (combat if ship, nav otherwise).

impaktor · 2024-11-26T11:51:42Z

I suspect most players don't even know about the 2D vs 3D radar.

bszlrd · 2024-11-26T12:10:53Z

Yeah, that button will definitely help with the discoverability

mwerle · 2024-11-26T14:07:18Z

I suspect most players don't even know about the 2D vs 3D radar.

I didn't.. (but then again I'm newly back to Pioneer..)

mwerle · 2024-11-27T23:39:09Z

add double-click to clear the navigation target

General thought - it might be better to have a "reject/unlock target" keybinding which unsets combat then navigation targets in that order. Nav/combat target is generally a state which is managed (by the player) at a higher level than the radar widget.

Reverted deselecting nav-target by double-click in 77e96f9

sturnclaw · 2024-11-28T00:01:33Z

EDIT: Some additional testing - if both Combat and Nav targets are set, 'Y' will disable the Combat target first, and then the Nav target. So this already (mostly) works as you suggest - but if there is a target over the reticule, it is set as the target instead (combat if ship, nav otherwise).

I'd like the deselect part of this to be duplicated with an additional keybind for "Unlock Target" (in addition to tapping the "Lock Target" keybind over empty space) - is that something you'd like to tackle, or should I make an issue for later work?

mwerle · 2024-11-28T00:30:42Z

EDIT: Some additional testing - if both Combat and Nav targets are set, 'Y' will disable the Combat target first, and then the Nav target. So this already (mostly) works as you suggest - but if there is a target over the reticule, it is set as the target instead (combat if ship, nav otherwise).

I'd like the deselect part of this to be duplicated with an additional keybind for "Unlock Target" (in addition to tapping the "Lock Target" keybind over empty space) - is that something you'd like to tackle, or should I make an issue for later work?

I can do it. Do you think it belongs in the radar module and this PR though?
I also want to add keybinds to toggle through all radar targets... at the moment there's only a keybind to toggle through "hostiles". Could be a tad tricky because toggling through ships ("potential hostiles") is different than nav targets - and should nav targets be restricted to "dockables"? It probably requires a bit more thought and is a bit beyond the scope of what I was aiming for here.

sturnclaw · 2024-11-28T00:49:59Z

I can do it. Do you think it belongs in the radar module and this PR though?

Probably not. I had only brought it up because I thought you might want to ensure the functionality to deselect an explicitly-selected target existed when the PR was merged, though if you want to tackle it in a follow-up PR please feel free.

mwerle · 2024-11-28T02:40:30Z

I can do it. Do you think it belongs in the radar module and this PR though?

Probably not. I had only brought it up because I thought you might want to ensure the functionality to deselect an explicitly-selected target existed when the PR was merged, though if you want to tackle it in a follow-up PR please feel free.

It already exists.. pressing 'Y' (default) deselects whatever has been selected. I personally would prefer to have a mouse-option on the radar given there's now a mouse option to select a target on the radar as well, but I understand that this is not the core functionality of the radar.

<- not a UI/UX engineer..

Anyway, let's punt this into a future PR.. should really do a major review of the UI/UX as a whole, define what functionality we want, and then come up with a unified control scheme.. ideally everything should be usable from keyboard/joystick with mouse optional.. (future VR support..?) But I think currently we are pretty far away from that goal.

sturnclaw

Good work! Thank you for putting up with my pedantic review as always, and for fixing that issue in Sound!

sturnclaw · 2024-11-30T06:48:21Z

Per discussion on IRC, the PR needs some method of deselecting the selected target via the radar interface - the easiest to discover method is to select the current target again (c.f. the behavior of labels in the WorldView when you click the targeted object again). As the diff is small, no re-approval is necessary before merge.

- Rather than doing sensible maths with if statements and multiplication, use the dark art of logarithms and exponents to compute the next/previous zoom setting for the radar. - Because all components of the zoom sequence repeat over an interval divisible by 10, we can operate on the log10 form of the zoom value by changing the fractional part of the number.

The 2D radar is trivial as it is a circle, but the 3D scanner is an ellipse. For now we check if the mouse is in a rectangle surrounding the scanner, which is an improvement but not a complete fix. TODO: if the mouse leaves the radar area while the popup for selecting between the radar modes is displayed, the popup disappears. The mouse-area-check should be disabled while the popup is displayed.

When the radar selection popup is open and the mouse leaves the radar area, the popup automatically closes even if the mouse is over the popup. Detect when the popup is open and only close it after it has been clicked on.

Left-clicking on a "pip" sets it as the combat target if it is a ship, otherwise as the navigation target. If the "pip" is the current target, the target is cleared instead. When there are multiple targets represented by a single "pip", a target selection popup is shown allowing the playing to select the correct target. Fix an issue if the player clicks outside a popup which could get the mouse handling confused and prevent future interactions with the radar. Also hide the "pip" tooltip if the target selection popup is active.

Move window size defintions near top of function so the sizes can be reused when calculating the bounds for detecting mouse clicks.

mwerle mentioned this pull request Nov 25, 2024

feat(radar): capture mouse wheel for zooming radar #5978

Merged

bszlrd added Cosmetic or UI New feature Cleanup labels Nov 25, 2024

mwerle added 3 commits November 25, 2024 17:45

fix(music): swap parameters

839e7af

Commit 678d46e added crossfading, but accidentally swapped two parameters when invoking "PlayMusic()".

feat(radar): ensure targeted object remains visible on manual zoom

2c92249

Ensure that a targeted object will always remain visible on the radar when switching from automatic to manual zoom.

feat(radar): logarithmic manual zoom levels

c79230c

Instead of increasing/decreasing the manual zoom level by powers of 10, use a semi-logarithmic scale of 1,2,5,10... when manually zooming in or out.

mwerle force-pushed the feat/radar_improvements branch from d16c081 to 229c3aa Compare November 25, 2024 08:45

mwerle commented Nov 25, 2024

View reviewed changes

data/pigui/modules/radar.lua Outdated Show resolved Hide resolved

mwerle marked this pull request as ready for review November 26, 2024 00:12

mwerle commented Nov 26, 2024

View reviewed changes

data/pigui/modules/radar.lua Outdated Show resolved Hide resolved

mwerle commented Nov 26, 2024

View reviewed changes

data/pigui/modules/radar.lua Show resolved Hide resolved

sturnclaw reviewed Nov 26, 2024

View reviewed changes

sturnclaw approved these changes Nov 30, 2024

View reviewed changes

sturnclaw and others added 5 commits November 30, 2024 16:14

feat(radar): fix radar selection popup disappearing

e04365d

When the radar selection popup is open and the mouse leaves the radar area, the popup automatically closes even if the mouse is over the popup. Detect when the popup is open and only close it after it has been clicked on.

feat(radar): remove right-click menu and code cleanup

9d0709c

Move window size defintions near top of function so the sizes can be reused when calculating the bounds for detecting mouse clicks.

mwerle force-pushed the feat/radar_improvements branch from cf2455d to 9d0709c Compare November 30, 2024 07:33

sturnclaw merged commit aaa9cb7 into pioneerspacesim:master Nov 30, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/radar improvements #5984

Feat/radar improvements #5984

mwerle commented Nov 25, 2024 •

edited

Loading

mwerle Nov 26, 2024

sturnclaw Nov 26, 2024

mwerle Nov 26, 2024

mwerle Nov 26, 2024

sturnclaw Nov 27, 2024

mwerle Nov 27, 2024 •

edited

Loading

sturnclaw Nov 28, 2024

mwerle Nov 30, 2024

sturnclaw Nov 30, 2024 •

edited

Loading

sturnclaw Nov 30, 2024

sturnclaw left a comment

sturnclaw Nov 26, 2024

mwerle Nov 26, 2024

sturnclaw Nov 27, 2024

mwerle Nov 27, 2024

mwerle Nov 28, 2024

sturnclaw Nov 28, 2024

sturnclaw Nov 26, 2024

mwerle Nov 26, 2024

mwerle commented Nov 26, 2024 •

edited

Loading

impaktor commented Nov 26, 2024

bszlrd commented Nov 26, 2024

mwerle commented Nov 26, 2024

mwerle commented Nov 27, 2024

sturnclaw commented Nov 28, 2024

mwerle commented Nov 28, 2024

sturnclaw commented Nov 28, 2024

mwerle commented Nov 28, 2024

sturnclaw left a comment

sturnclaw commented Nov 30, 2024

Feat/radar improvements #5984

Feat/radar improvements #5984

Conversation

mwerle commented Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwerle Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sturnclaw Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sturnclaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwerle commented Nov 26, 2024 • edited Loading

impaktor commented Nov 26, 2024

bszlrd commented Nov 26, 2024

mwerle commented Nov 26, 2024

mwerle commented Nov 27, 2024

sturnclaw commented Nov 28, 2024

mwerle commented Nov 28, 2024

sturnclaw commented Nov 28, 2024

mwerle commented Nov 28, 2024

sturnclaw left a comment

Choose a reason for hiding this comment

sturnclaw commented Nov 30, 2024

mwerle commented Nov 25, 2024 •

edited

Loading

mwerle Nov 27, 2024 •

edited

Loading

sturnclaw Nov 30, 2024 •

edited

Loading

mwerle commented Nov 26, 2024 •

edited

Loading