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.

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

Feat/radar improvements #5984

Merged
merged 8 commits into from
Nov 30, 2024

Conversation

mwerle
Copy link
Contributor

@mwerle mwerle commented Nov 25, 2024

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


  1. ensure targeted object remains visible when switching from automatic to manual zoom (feat(radar): capture mouse wheel for zooming radar #5978 (review))
  2. add intermediate zoom steps for manual zoom (feat(radar): capture mouse wheel for zooming radar #5978 (review))
  3. fix popup disappearing when mouse moves outside of radar area but still over popup
  4. improve checking when mouse is over radar area (especially for the 3D scanner mode display)
  5. 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".
  6. 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.
@mwerle mwerle force-pushed the feat/radar_improvements branch from d16c081 to 229c3aa Compare November 25, 2024 08:45
data/pigui/modules/radar.lua Outdated Show resolved Hide resolved
@mwerle mwerle marked this pull request as ready for review November 26, 2024 00:12
Comment on lines 86 to 115
-- 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"..

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@mwerle mwerle Nov 27, 2024

Choose a reason for hiding this comment

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

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..

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@sturnclaw sturnclaw Nov 30, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
Copy link
Member

@sturnclaw sturnclaw left a comment

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

Ships are always combat targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ships are always combat targets.

Not really - rescue missions, for example.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 0665cc5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
@@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

Good catch, thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

mwerle commented Nov 26, 2024

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
Copy link
Member

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

@bszlrd
Copy link
Contributor

bszlrd commented Nov 26, 2024

Yeah, that button will definitely help with the discoverability

@mwerle
Copy link
Contributor Author

mwerle commented Nov 26, 2024

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
Copy link
Contributor Author

mwerle commented Nov 27, 2024

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
Copy link
Member

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
Copy link
Contributor Author

mwerle commented Nov 28, 2024

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
Copy link
Member

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
Copy link
Contributor Author

mwerle commented Nov 28, 2024

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.

Copy link
Member

@sturnclaw sturnclaw left a comment

Choose a reason for hiding this comment

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

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

@sturnclaw
Copy link
Member

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.

sturnclaw and others added 5 commits November 30, 2024 16:14
- 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 mwerle force-pushed the feat/radar_improvements branch from cf2455d to 9d0709c Compare November 30, 2024 07:33
@sturnclaw sturnclaw merged commit aaa9cb7 into pioneerspacesim:master Nov 30, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants