feat(ui): rework target scanner display#6181
feat(ui): rework target scanner display#6181sturnclaw merged 5 commits intopioneerspacesim:masterfrom
Conversation
|
Nice! |
|
if the orbit display is also a hud module, they'll automatically rearrange with each other according to priority. |
mwerle
force-pushed
the
feat/ui-target-scanner-display
branch
from
38e97e0 to
2e82fc8
Compare
|
WiP : as part of this work, I've decided to also refactor the hyperspace cloud a little bit and moved the hyperspace origin into it, instead of overloading the ships' hyperspace destination. I've pushed this to a separate branch as I'm not sure it's entirely warranted here, and I don't want to hold this PR up unnecessarily, but if people are interested in taking a quick look, it's here : 64ab51e |
sturnclaw
left a comment
sturnclaw
left a comment
There was a problem hiding this comment.
Overall I'm quite happy with this PR, thank you for taking the initiative with it! I have a few nitpicks and places where I'd like to see some improvements, most notable of which is better differentiating where the hypercloud analyzer display ends and the target scanner display begins:
Another thing I'd suggest is to at least display the type of ship "in" the departure/arrival cloud, since we're already displaying its mass. I think we can make the assumption that the analyzer has hyperspace "wake signatures" to compare against known models of ships.
mwerle
force-pushed
the
feat/ui-target-scanner-display
branch
from
2e82fc8 to
bc191f1
Compare
...
How's this : 
EDIT: There's a newer screenshot which restricts the width here : #6181 (comment) |
|
I think I addressed all comments. Please re-review at your leisure. (Will squash the various fixup commits once accepted prior to merge) |
I've done this for now, but in future perhaps we can have different versions of the Hyperspace Cloud Analyzer, giving different amounts of information. Then again, could just lead to unnecessary equipment creep. |
|
I think that kind of equipment creep would be a good thing. Especially when the syístems become more complex. Like one sensor could have lower range and less data, but cheaper and available at lower tech levels. Another medium quality would be at higher tech levels, would have more range, and could guess the ship size 50% of the time, and the best version would guess 90% of the time at higher ranges, but is a very sofisticated instrument. Power and maintenance needs could also differ. |
I generally agree that having additional equipment options is a good thing, as long as those options represent meaningful choices for the player. If the "best option" has no cost other than money, players will only ever fit something else as a temporary stopgap. |
sturnclaw
left a comment
sturnclaw
left a comment
There was a problem hiding this comment.
One minor change that I'd like to see addressed regarding the addition of the gray_800 color breakpoint, but otherwise I'm exceptionally happy with this PR!
data/pigui/themes/default.lua
Outdated
| notificationBackground = styleColors.panel_800, | ||
| modalBackground = styleColors.panel_900, | ||
| tableBackground = styleColors.primary_900, | ||
| tableHeaderBg = styleColors.gray_800, |
There was a problem hiding this comment.
The panel set of semantic colors is intended to be used as background colors, while the gray set are foreground colors (for e.g. text elements). In this particular case, I'd strongly recommend using panel_800 for consistency with the rest of the UI instead.
The inability to directly theme the TableHeaderBg color is something that can be addressed by adding the enum definition to src/lua/LuaPiGui.cpp in the imgui_col_enums definition.
There was a problem hiding this comment.
The problem is that no currently defined color matches the color used in the table header row. This appears to only be implemented in the C++ at the moment, which is why I created a new color in the LUA to mimic it.
What, if anything, do the numbers in the color definitions mean anyway?
There was a problem hiding this comment.
This is using panel_800 - as you can see, it's a different grey to the one used in the actual table header.
For context, the header for the "hyperspace cloud analyzer" is drawn as a filled background rectangle and then a text line as I can't figure out a way to have a table row span multiple columns.
The "target scanner" header is a table header row, rendered using the ImGuiCol_Header colour. This is nominally exposed to LUA as the Header colour, but the current theme has it as a purple colour.
So this is probably a bug that the theme affects the LUA side, but not the C++ side..
When using colors.Header:
There was a problem hiding this comment.
@sturnclaw ? (or anybody else with knowledge of how this works..)
There was a problem hiding this comment.
Numbers in color definitions are the color "value", loosely based on a semantic color system. They provide a categorization system to ensure that colors with the same value number appear relatively consistent across the entire UI.
The inability to theme the table header color is because the table header row doesn't use the Header color at all, but rather the TableHeaderBg color (see imgui_tables.cpp in the Imgui::TableBeginRow() function). That color is not yet exposed to Lua; to do so, you'd need to add a new entry to the imgui_col_enums table in src/lua/LuaPiGui.cpp, starting on line 254.
Once that color has been added, it should be normalized with our theme by modifying data/pigui/themes/default.lua to add TableHeaderBg = styleColors.panel_800, somewhere around line 240.
There was a problem hiding this comment.
As nobody has answered I still have no clue what the numbers in these theme color basenames are supposed to mean.
No one (that I've been able to find) really knows what the numbers mean precisely, they're just a rough categorical scale from 000=white to 999=black. You can kind of think of the value as a Z-index; UI elements with a smaller color value number go on top / inside of elements with a greater number (until we start talking about light themes, in which case it inverts).
The linked article goes into the philosophy of the color system we've (mostly) adopted, but the convention of using a three-digit suffix to indicate relative color value is a web design thing going back at least a decade to my knowledge.
There was a problem hiding this comment.
I had a quick read through that article (thank you). IMHO it might have been useful to link that at the top of the themes/default.lua file..
There was a problem hiding this comment.
The inability to directly theme the TableHeaderBg color is something that can be addressed by adding the enum definition to src/lua/LuaPiGui.cpp in the imgui_col_enums definition.
And my apologies, I missed / didn't understand this part of your original comment..
There was a problem hiding this comment.
I had a quick read through that article (thank you). IMHO it might have been useful to link that at the top of the
themes/default.luafile..
I keep forgetting to do so every time I do a major theme update. If you don't intend to add such to this PR, I'll push it as a commit to master after merge.
There was a problem hiding this comment.
I can quickly add it.. Give me a minute.
mwerle
force-pushed
the
feat/ui-target-scanner-display
branch
3 times, most recently
from
85b8a8f to
d49bf8b
Compare
sturnclaw
left a comment
sturnclaw
left a comment
There was a problem hiding this comment.
Looks good to me!
We historically don't maintain our own "copies" of deprecated ImGui functions or enum values, and instead perform a migration to the new value once ImGui fully removes the deprecated value in an upstream update. That being said, there's no rule against maintaining a "deprecated list" in LuaPiGui, and I don't consider it to have any bearing on merging this PR.
The deprecated colour value was already in the table and I was too lazy to see if it was being used anywhere, so didn't want to remove it and potentially break something... and ImGui hasn't fully removed it yet. At least, depending on whether or not I did a |
The perfectionist in me would appreciate it - but it's not required. |
Update the LUA ImGui color definition enum table to match the current ImGui definitions. Presumably this table was not updated following an upgrade of the DearImGui library, or alternatively, these missing entries were never defined in the past as they weren't being used. In either case, ensure that the LUA side has full access to the color definitions on the C++ side.
Possibly we should add all of the ImGui style colors into the LUA theme. For now just add the TableHeaderBg value as that is required to color-match some display elements for the rewritten target scanner module.
The hyperspace cloud label was not updated when a ship is evicted from it.
Partially fixes pioneerspacesim#6162 Convert it to a HudModule - this means it will be automatically hidden when a sidebar module is opened. The way the data for both ship details and hyperspace clouds are rendered has been cleaned up and modernised.
As suggested by Sturnclaw, the hyperspace cloud analyzer should be able to determine and show the ship type.
mwerle
force-pushed
the
feat/ui-target-scanner-display
branch
from
d49bf8b to
87bdf95
Compare
3 participants
Partially fixes #6162
Fixes #5751
Convert it to a HudModule - this means it will be automatically hidden when a sidebar module is opened.
The way the data for both ship details and hyperspace clouds are rendered has been cleaned up and modernised.
Screenshots:
Old:
Overlapping with right-hand sidebar module:
New:
No more overlapping (auto-hide):
