feat(radar): add manual zoom mode to scanner #5958

mwerle · 2024-11-06T11:11:06Z

The radar is zoomed manually, but the scanner has automatic zoom which can lead to some issues while trying to get an overview of the current area around the ship as it is usually zoomed out too far.

This commit adds key bindings to switch between the radar and the scanner, as well as bindings to zoom the scanner and reset the mode back to automatic.

The zoom bar of the scanner is moved up to make room for the new manual/automatic zoom mode indicator on the left as well as the current range indicator which is in the same position on the right as the radar display.

To make the display less cluttered, the left and right button bars were moved away from the center slightly, they now end and start, respectively, on the edges of the radar area.

TODO:

add hover-mode for the scanner to show ship information
use keybindings instead of hardcoded keys
use icons for the automatic/manual mode indicator
- get final icon glyphs - THANK YOU @bszlrd
get buttons actually working
(possibly?) use the new Input system to register and use keybindings
code review
clean up git history before merge

Auto-zoom:

Manual Zoom:

impaktor · 2024-11-06T11:35:26Z

This works for 2D azimutal radar as well? (Right click the radar)

mwerle · 2024-11-06T12:42:06Z

This works for 2D azimutal radar as well? (Right click the radar)

Yes - in fact, the radar is only manual zoom. It already could be zoomed using the mouse scroll-wheel, and, now, using the zoom keys I defined. I haven't added an auto-zoom mode to it (yet.. is it desired?)

The mouse scroll-wheel has the issue that it isn't captured - the event is passed to the rest of the game engine and causes the view to zoom as well.

bszlrd · 2024-11-06T16:29:47Z

How about a button like this? Not yet sure about the icons though, the A-M on them is kind of a cop-out, but I have to figure out a good iconography for it.

bszlrd · 2024-11-07T05:22:21Z

How about icons like this?

mwerle · 2024-11-07T07:03:43Z

How about icons like this?

Might have to see them "in the flesh", but I suspect that they will be too "busy" at the size we need.

I was thinking [AF] / [MF] or just [A]/[M] as per common camera icons, for example:

https://thenounproject.com/browse/icons/term/manual-camera-mode/

bszlrd · 2024-11-07T15:12:23Z

I'll put them in flesh tomorrow evening, and we can see how busy they would be.

bszlrd · 2024-11-07T16:38:08Z

Been able to sneak in some time while waiting. How about this?

Or this? I prefer this one, with the radar mode switcher, and the AM button next to it, when it is on planar. Helps with the discoverability of the azimuthal radar. I think a lot of people don't know about it.

mwerle · 2024-11-07T23:07:13Z

Been able to sneak in some time while waiting. How about this?

I quite like the first A/M icons! But they'd be a bit big I guess in combination with the switcher icon unless we distribute the icons around the scanner instead of just having them at the bottom. Eg, have the switcher at the top-left?

Personally I think the switcher-icon is a little bit abstract (I know it's a representation of the different scanner visuals) and making the A/M icon that small .. I'd have to see it in-game I guess.

That said, I'm not a UI person; technically I'm getting close to having it work and I'll leave the iconography up to you / everybody else to vote on.

Thank you for the mockups!!

bszlrd · 2024-11-08T17:57:04Z

I'd rather not put any buttons above the scanner. I think it is more cluttered, disorganized that way:

There could be two proper buttons, next to the thruster feedback box. Like this if we don't mind that they are 36px instead of 40:

Or rather we could shring all the buttons to 36px (I kept the size of the thruster box). I think I prefer this one, no need for that large buttons:

Hmm, this could also allow for a +- button for setting range (but I'd also put that on the mousewheel over the scanner itself:

And on that note: first I thought that the range would be irrelevant for the azimuthal mode, but the more I think about it the more it seems that it would still be useful for that. As a cut-off distance: the ships further than that aren't shown. Could be useful for combat too, to reduce clutter.

mwerle · 2024-11-08T22:49:09Z

The mouse-wheel already is used for zoom (although, as I said, it clashes with the main-viewport zoom functionality so is kindof broken at present).

I'm not a fan of adding a button for every possible function to the main display; it's already cluttered.. but ultimately, I'll leave the UI design up to other people as I have limited understanding of UI design... . I'm more interested in the actual functionality.

So whatever you think looks and works best :)

mwerle · 2024-11-08T23:55:37Z

Changed the "radar" to match the "scanner" colours. (Sorry, "azimuthal" is just not something which rolls off the tongue or which the average gamer can be expected to understand, imho)

mwerle · 2024-11-11T13:59:33Z

I've been tweaking the implementation a little bit and managed to figure out how to use bound keys in the Lua. Still need to decide on a good set of defaults (or even none).

One final issue I can't crack - actually getting the buttons to work.
For some reason, the buttons don't react to the mouse at all, nor do the tooltips show up. If I put the entire radar into a ui.window() (as per other display elements), then the 3d radar background doesn't get rendered (the "pips" do, and in the expected location) nor do the buttons (although I may have stuffed up some coordinate calculations for the buttons).

But experimenting with creating a separate small window with a button inside the radar's draw routine had a properly-working button with tooltip and reacting to the mouse.

Once I get the buttons working (help?), and some button glyphs from @bszlrd I think this PR is getting close to being ready for review.

bszlrd · 2024-11-11T14:01:08Z

I'll get to it, I think I will be able to tonight

mwerle · 2024-11-11T14:03:18Z

I'll get to it, I think I will be able to tonight

Thank you! But no rush :) I still can't get the bleeding buttons to actually work...

bszlrd · 2024-11-11T14:04:46Z

Mabye the scanner overlaps and blocks them?

mwerle · 2024-11-11T14:05:33Z

Here's the latest WiP screenshot:

mwerle · 2024-11-11T14:06:42Z

Mabye the scanner overlaps and blocks them?

The text-label for the zoom works properly with a tooltip.. and it (was) way more overlapped than the buttons. I have no idea..

bszlrd · 2024-11-11T15:09:26Z

Here are the icons:

icons.zip
I've added them to the last two free spaces at the end. You still need to set them up in data/pigui/themes/default.lua
I did not yet add the A-M icons, I'm assuming you want to show them by text. If so, then I'd suggest using the Orbiteer font for that.

mwerle · 2024-11-12T01:21:30Z

Here are the icons:

Thanks! You should add the commit to the PR directly in order to be accredited for your work :)

Actually the A/M icons would be more useful right now - I'm thinking that unless someone can help me fix the toggle button, it's best not to display that at all. It's kindof pointless unless it works, whereas the A/M indicator is important feedback and, as it's just an indicator, does not need functionality (although it would be nice to have a tooltip at least..).

I can also revert to just using text directly if you think it's better than using icons.

mwerle · 2024-11-12T08:14:48Z

Hmm.. they seem a bit small and difficult to make out given the button size. And I have a pretty big monitor.

What do you think?

bszlrd · 2024-11-12T09:41:48Z

Enlarged and thickened the mode icons, and added zoom icons (added a new row for those)
icons.zip

mwerle · 2024-11-12T09:42:57Z

I replaced the other button with text using the "orbiteer" font.

mwerle · 2024-11-12T12:02:16Z

mwerle · 2024-11-13T01:12:49Z

Ok, I think this is close to being ready for code review.. Git history needs cleaning up before merge.

impaktor · 2024-11-13T11:00:22Z

Hmm.. they seem a bit small and difficult to make out given the button size.

Is there tooltip / mouse over? Then we could be satisfied with this as first iteration.

mwerle · 2024-11-13T13:10:48Z

Rebased the commits.

I left commit be5bd23 as a separate commit instead of squashing it as I think it could be a useful reference for anybody else converting hard-coded Lua keys to the Input system, although I guess that will change with the new first-class Lua Input system.

If anybody is interested in the messy details of how this feature was achieved, I tagged the original series of 18 (!) commits here : https://github.com/mwerle/pioneer/commits/radar_manual_zoom_1/

bszlrd · 2024-11-13T13:15:56Z

If the icons aren't readable, I can easily play with them after it is merged, and open a PR for them later on.

sturnclaw

Gave this a quick read-through and left comments/suggestions where appropriate. The keybinding names should a) be somewhat short, and b) should be nouns rather than sentences if possible. We can look at adding description strings to the keybindings if needed for clarity.

Overall, the new buttons for the radar mode are implemented in a very sane manner (by the standards of this codebase) and they function reasonably well at smaller screen sizes - except for the A/M icons.

There is a bit of overlap at 1280x720 however, as the HUD window draws on top of the rest of the Flight UI. I'll consider that to be a task for another PR, as that's a bit out-of-scope of this work.

I leave it to your discretion whether you want to merge this PR as is (after addressing review feedback) and move the Input bindings to Lua in a later PR, or wait until #5962 and the associated hooks for pushing InputFrames have been merged.

data/lang/input-core/en.json

sturnclaw · 2024-11-13T18:17:13Z

src/ship/PlayerShipController.cpp

@@ -166,6 +166,13 @@ REGISTER_INPUT_BINDING(PlayerShipController)
 	auto landingGroup = controlsPage->GetBindingGroup("LandingControl");
 	input->AddActionBinding("BindToggleLandingGear", landingGroup, Action({ SDLK_n }));
 	input->AddAxisBinding("BindControlLandingGear", landingGroup, Axis());
+
+	auto radarGroup = controlsPage->GetBindingGroup("RadarControl");


This should likely be in a new "Flight HUD" page rather than "Ship - Controls" - the latter is intended for bindings affecting the physical control state of the ship, while the new keybindings are related only to the radar widget and don't affect anything about the "in-universe" ship.

Ok; this may take a few days (have a conference the next 2 days, then hopefully away on a motorbike trip)

Fixed in e2b015d

data/pigui/modules/radar.lua

src/ship/PlayerShipController.cpp

sturnclaw · 2024-11-13T18:40:08Z

src/ship/PlayerShipController.cpp

+
+	AddAction("BindRadarToggleMode");
+	AddAction("BindRadarZoomReset");
+	// TODO: Convert to axis?


As mentioned above, I'd recommend adding an additional axis which is only activated when hovering the radar widget. Future code could also make the axis "active" when the radar widget has received some sort of input focus, for use with e.g. controllers.

Axis partially added in d0e8491 ; will need to investigate how this can be activated on mouse-over.

Axis removed again until I can fully implement it.

sturnclaw · 2024-11-13T18:40:41Z

data/pigui/modules/radar.lua

+	ui.addCircle(center, self.size, fgColor, ui.circleSegments(self.size), lineThickness * 2)
+
+	-- inner circles
+	--	ui.addCircle(center, halfsize, fgColor, ui.circleSegments(halfsize), lineThickness)


These were in the original code and add additional lines to the 2D radar. I left them in as they may want to be enabled again in a future UI revision?

Go ahead and leave them in then, but leave a comment inline with them mentioning the above please.

Merged into commit 81a7203

sturnclaw · 2024-11-13T18:40:45Z

data/pigui/modules/radar.lua


 	local function line(x,y)
-		-- ui.addLine(cntr + Vector2(x, y) * halfsize, cntr + Vector2(x,y) * size, colors.reticuleCircle, ui.reticuleCircleThickness)
-		ui.addLine(cntr + Vector2(x, y) * thirdsize, cntr + Vector2(x,y) * twothirdsize, colors.reticuleCircle, ui.reticuleCircleThickness)
+		-- ui.addLine(center + Vector2(x, y) * halfsize, center + Vector2(x,y) * size, colors.uiPrimaryDark, lineThickness)


As per #5958 (comment)

bszlrd · 2024-11-13T21:25:03Z

@Web-eWorks noticed an issue with some other icons in the previous file (the warning !!!s). I've fixed them in this one:
icons.zip

mwerle · 2024-11-13T22:02:48Z

@Web-eWorks noticed an issue with some other icons in the previous file (the warning !!!s). I've fixed them in this one: icons.zip

Thanks! Committed in acd89d8

mwerle · 2024-11-13T22:07:58Z

Overall, the new buttons for the radar mode are implemented in a very sane manner (by the standards of this codebase) and they function reasonably well at smaller screen sizes - except for the A/M icons.

Hmm; will need to take a look to see what can be done here, but it'll take a few days as I'm away for the next few.

I leave it to your discretion whether you want to merge this PR as is (after addressing review feedback) and move the Input bindings to Lua in a later PR, or wait until #5962 and the associated hooks for pushing InputFrames have been merged.

There's a couple of other fixes which I likely won't get around to for a few days, so I can rewrite the input to use #5962 if that has been merged by then. Probably not a bad idea to showcase the new system.

sturnclaw · 2024-11-13T22:38:50Z

Hmm; will need to take a look to see what can be done here, but it'll take a few days as I'm away for the next few.

I've asked @bszlrd to increase the size of the icons themselves to occupy the full icon square which will help, and then it's just a question of how large the button is at low screen resolutions.

There's a couple of other fixes which I likely won't get around to for a few days

Take your time! Enjoy the weekend 😄

bszlrd · 2024-11-14T06:16:30Z

Of course I forgot to increase the sizes of AM :D

bszlrd · 2024-11-14T16:14:05Z

Fixed the AM icons too:
icons.zip

mwerle · 2024-11-15T02:42:59Z

I leave it to your discretion whether you want to merge this PR as is (after addressing review feedback) and move the Input bindings to Lua in a later PR, or wait until #5962 and the associated hooks for pushing InputFrames have been merged.

Fixed in e2b015d

Squashed fix into 9ec571d

mwerle · 2024-11-15T02:44:36Z

Fixed the AM icons too: icons.zip

Merged into 3d54882 -> fixup commit replacing the original commit.

sturnclaw

Provisionally approved - I'll give it a playtest soon ™️.

data/pigui/modules/radar.lua

sturnclaw · 2024-11-15T18:10:18Z

src/lua/LuaGame.cpp

@@ -776,6 +816,7 @@ void LuaGame::Register()

 		{ "SwitchView", l_game_switch_view },
 		{ "CurrentView", l_game_current_view },
+		{ "PreviousView", l_game_previous_view },


I would generally prefer to add additional events fired by a View when it is made (in)active than introduce this new method - though if pushing/popping the InputFrame is implemented according to #5958 (comment) neither should be necessary.

Hmm.. ok, getting very mixed signals (pun not intended) here.. in another PR you wanted me to remove events and use function calls. Here you're saying I should be using events.

What paradigm is this codebase supposed to use?!

Sorry about the confusion - there are three (now four) possible states to implement the functionality of popping the input frame:

The simplest way would be to statically call RemoveFromStack every time onViewChanged is triggered, regardless of if the input frame is active. (I can add an Input.HasInputFrame(frame) method if you're concerned about the performance here.)

Pass the name of the previous view as a parameter to the onViewChanged event.

If option 1 or 2 is not chosen, I'd prefer PiGuiView queue an onViewActivated / onViewDeactivated event.

The currently-implemented method of Game.PreviousView, which adds an additional API call and duplicates the view name lookup code.

I had originally suggested option 1 in #5958 (comment), with this comment expressing my desire to use option 3 instead of 4 if option 1 was not chosen. Upon reflection, I think option 2 would be the simplest possible way to achieve this, while still being performant.

At this point - just pick whatever you want to implement; I've nitpicked the PR to death and you're more than welcome to say "not going to bother" to nitpicks :)

Lol, I actual thought of "2" while I was reverting.. I'll leave it as-is for now, but adding "2" at some point would be a good addition I think. That'd be the most performant and least invasive.

sturnclaw

As before, provisionally approved - I should get a chance to playtest this tomorrow morning.

The radar is zoomed manually, but the scanner has automatic zoom which can lead to some issues while trying to get an overview of the current area around the ship as it is usually zoomed out too far. This commit adds key bindings to switch between the radar and the scanner, as well as bindings to zoom the scanner and reset the mode back to automatic. The zoom bar of the scanner is moved up to make room for the new manual/automatic zoom mode indicator on the left as well as the current range indicator which is in the same position on the right as the radar display. The alternative is to display the range in the center above the zoom bar, but then it is in a different position compared to the radar. TODO: * use icons for the automatic/manual mode indicator * add hover-mode for the scanner to show ship information * use keybindings instead of hardcoded keys

Create key bindings to use for controlling the radar. The key bindings are specified in the C++ side and the accessed from Lua. The user can modify the binds using the options page.

The radar ("azimuthal mode") is drawn using the reticule colours (white), which clashes with the rest of the dashboard icons/controls as well as the scanner ("3d mode"). This commit changes the radar colours to (mostly) match the scanner colours. It also tweaks the line-width of the scanner and radar backgrounds: - rader inner lines are made thinner - scanner border is made thicker

Szlrd created icons both for the scanner/radar toggle button as well as for the manual/automatic zoom mode indicator button. Two of the new icons are on the 21st row (starting on index 320) for the radar's automatic and manual zoom mode indicator. This necessitated modifying several other files to reflect the increased grid size of the icons.svg file. Co-authored-by: Szlrd <[email protected]>

This commit is a major refactor of the radar module code. The scanner and radar were each turned into a class (table) to make the main draw function more generic. TODO: there's probably a good way to refactor this further to have both instruments derive from a base class in order to ensure the same base functionality. The layout was also modified significantly with both the left and right button bars moved further away for an overall less cluttered look. This made room for a couple of new buttons, one to switch between the scanner and the radar, and the other to indicate the current scanner zoom mode - automatic or manual. In order to get the new buttons near the radar to actually work, they had to be rendered inside a window as otherwise they are completely unresponsive the mouse. HOWEVER, placing the entire randar into the window does not work as the 3D radar's background is not rendered when it's in the window (possibly due to coordinate issues? Alternatively because it's rendered in C++ instead of LUA?) although its "pips" are. The 2D radar is rendered properly even when inside a window. Additionally: * The 'onChangeMFD' event was removed * The scanner (3D) and radar (2D azimuth) now each have their own zoom level. This prevents the radar from ending up with an auto-zoom zoom level when switching from the scanner since it only ever has manual zoom. * The ship-internals window is moved to the left a bit to avoid it encroaching on the radar area. * The autopilot controls are moved to the right a little bit to make more room for the zoom-level display. * The new buttons for the radar are aligned to the left of the radar area. * The scanner was made marginally taller. * The zoom levels are reset for both the scanner and the radar at game-end. * Add a debugReload() function to the ship-internals window.

Move the input control registration from C++ to Lua. From a user-perspective, the move also includes moving the configuration into a new "Ship HUD" controls page, as well as changing the default key bindings away from the arrow-keys to avoid clashing with the radial menu popups. TODO: Add an axis to control the radar zoom level.

mwerle force-pushed the feat/radar_manual_zoom branch from d3be2fa to 62b0501 Compare November 8, 2024 23:39

mwerle force-pushed the feat/radar_manual_zoom branch from 62b0501 to 7e2b067 Compare November 10, 2024 10:55

mwerle added Cosmetic or UI Lua code labels Nov 12, 2024

mwerle marked this pull request as ready for review November 13, 2024 01:13

sturnclaw self-requested a review November 13, 2024 18:08

sturnclaw requested changes Nov 13, 2024

View reviewed changes

mwerle force-pushed the feat/radar_manual_zoom branch from d0e8491 to e2b015d Compare November 15, 2024 02:41

sturnclaw approved these changes Nov 15, 2024

View reviewed changes

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

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

mwerle force-pushed the feat/radar_manual_zoom branch from 3d9193e to 9ec571d Compare November 15, 2024 08:35

sturnclaw requested changes Nov 15, 2024

View reviewed changes

sturnclaw approved these changes Nov 16, 2024

View reviewed changes

mwerle and others added 8 commits November 16, 2024 09:41

feat(radar): convert hard-coded keys to use bind-manager

9df947b

Create key bindings to use for controlling the radar. The key bindings are specified in the C++ side and the accessed from Lua. The user can modify the binds using the options page.

feat(radar): add tooltip for targets to scanner

6764af4

ui(hud): add translation for target radial key bind

24bb818

mwerle force-pushed the feat/radar_manual_zoom branch from 6769b3b to 24bb818 Compare November 16, 2024 00:41

sturnclaw merged commit b928668 into pioneerspacesim:master Nov 16, 2024
4 checks passed

mwerle deleted the feat/radar_manual_zoom branch November 17, 2024 08:46

This was referenced Nov 18, 2024

Feat/view event and names #5974

Merged

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

Merged

feat(radar): add manual zoom mode to scanner #5958

feat(radar): add manual zoom mode to scanner #5958

Conversation

mwerle commented Nov 6, 2024 • edited by sturnclaw Loading

impaktor commented Nov 6, 2024

mwerle commented Nov 6, 2024

bszlrd commented Nov 6, 2024

bszlrd commented Nov 7, 2024

mwerle commented Nov 7, 2024 • edited Loading

bszlrd commented Nov 7, 2024

bszlrd commented Nov 7, 2024

mwerle commented Nov 7, 2024

bszlrd commented Nov 8, 2024 • edited Loading

Hmm, this could also allow for a +- button for setting range (but I'd also put that on the mousewheel over the scanner itself:

mwerle commented Nov 8, 2024

mwerle commented Nov 8, 2024

mwerle commented Nov 11, 2024 • edited Loading

bszlrd commented Nov 11, 2024

mwerle commented Nov 11, 2024

bszlrd commented Nov 11, 2024

mwerle commented Nov 11, 2024

mwerle commented Nov 11, 2024

bszlrd commented Nov 11, 2024

mwerle commented Nov 12, 2024 • edited Loading

mwerle commented Nov 12, 2024

bszlrd commented Nov 12, 2024

mwerle commented Nov 12, 2024

mwerle commented Nov 12, 2024

mwerle commented Nov 13, 2024

impaktor commented Nov 13, 2024

mwerle commented Nov 13, 2024 • edited Loading

bszlrd commented Nov 13, 2024

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

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

bszlrd commented Nov 13, 2024

mwerle commented Nov 13, 2024

mwerle commented Nov 13, 2024

sturnclaw commented Nov 13, 2024

bszlrd commented Nov 14, 2024

bszlrd commented Nov 14, 2024

mwerle commented Nov 15, 2024 • edited Loading

mwerle commented Nov 15, 2024

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

sturnclaw Nov 16, 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 left a comment

Choose a reason for hiding this comment

mwerle commented Nov 6, 2024 •

edited by sturnclaw

Loading

mwerle commented Nov 7, 2024 •

edited

Loading

bszlrd commented Nov 8, 2024 •

edited

Loading

mwerle commented Nov 11, 2024 •

edited

Loading

mwerle commented Nov 12, 2024 •

edited

Loading

mwerle commented Nov 13, 2024 •

edited

Loading

mwerle commented Nov 15, 2024 •

edited

Loading

sturnclaw Nov 16, 2024 •

edited

Loading