Compare specs of your current ship to those in the ship market and highlight changes#5632
Compare specs of your current ship to those in the ship market and highlight changes#5632sturnclaw merged 10 commits intopioneerspacesim:masterfrom
Conversation
…ng at and display the stats colored to show the difference
…er/worse and custom padded columns so it all fits nicely
…nitions to the drawing class too
sturnclaw
left a comment
sturnclaw
left a comment
There was a problem hiding this comment.
Finally gotten around to reviewing this, been swamped / away from Github for the last little while.
Overall I'm very happy with this PR; it's not quite as compact a solution as I'd prefer in an "ideal world", but it's definitely a very clean and readable solution and not worth spending any further time over for the sake of code golf.
I've made a few suggestions for cleanliness / code smell / maintainability reasons; feel free to ask if you have any questions how to accomplish the needed changes!
…older for now) and adding the color and placeholder icons to the theme We also add an empty fixed width column and use existing column padding rather than doing custom padding with typographic spaces
|
@nozmajner would you have any time in your schedule to make "better" / "worse" arrow icons for the icon sheet? For a quick-and-dirty option, you could use the export major / import major icons and just remove the outer ring... |
|
I don't know when I can get around to it. I'd use one of the triangular icons for now instead |
|
I think I'd use either of these: |
|
|
|
Looks cool! |
sturnclaw
left a comment
sturnclaw
left a comment
There was a problem hiding this comment.
Looks good to me! I'll give this a test sometime in the next day or so and pull the merge trigger.
sturnclaw
left a comment
sturnclaw
left a comment
There was a problem hiding this comment.
After having tested it, I have an issue and a feature request:
- The forward acceleration is presented in negative G's - while I know this was probably existing behavior, it's rather nonsensical (and doesn't match the Ship Info screen). It also seems to be incorrectly comparing with the current ship, as a ship with a lower empty forward acceleration than my ship's current forward acceleration is showing up as having "better" acceleration.
- I'd like to see a tooltip when hovering stats that indicates the current value of that stat on your ship, as that would save the player a click into a different info screen every time they want to know exactly how much better the new ship's equipment is.
…ted forward accelerations. Add tooltips to the elements to compare and show the current values.
|
Here we are with tooltips |
sturnclaw
left a comment
sturnclaw
left a comment
There was a problem hiding this comment.
One last batch of mostly stylistic/code-quality nitpicks and this is good to merge, thanks for bearing with the long review process!
As mentioned on IRC, the documentation to FormatAndCompareShips:compare_and_draw_column declares fmt_a twice; that should be a quick cleanup while you're there.
* Add "Current Ship: " before the current ship values in the tooltip * Fix duplicated parameter name annotation * Remove some really nasty double negation logic for much cleaner result
sturnclaw
left a comment
sturnclaw
left a comment
There was a problem hiding this comment.
Looks good to me! You can manually clean up the git history, or I can squash-merge all the history into a single commit.
3 participants
When you are in the ship market we now color the different specifications and have an indicator arrow next to them to show if the ship being viewed is an improvement or not over the current one: