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

Compare specs of your current ship to those in the ship market and highlight changes #5632

Merged
merged 10 commits into from
Oct 8, 2023

Conversation

JonBooth78
Copy link
Contributor

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:

image

Copy link
Member

@Web-eWorks Web-eWorks left a comment

Choose a reason for hiding this comment

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

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!

data/pigui/modules/station-view/04-shipMarket.lua Outdated Show resolved Hide resolved
data/pigui/modules/station-view/04-shipMarket.lua Outdated Show resolved Hide resolved
…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
@Web-eWorks
Copy link
Member

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

@bszlrd
Copy link
Contributor

bszlrd commented Sep 28, 2023

I don't know when I can get around to it. I'd use one of the triangular icons for now instead

@bszlrd
Copy link
Contributor

bszlrd commented Sep 28, 2023

I think I'd use either of these:
image

@JonBooth78
Copy link
Contributor Author

screenshot-20231003-194811
I changed the icon set and we now end up with the above.

@bszlrd
Copy link
Contributor

bszlrd commented Oct 3, 2023

Looks cool!

Copy link
Member

@Web-eWorks Web-eWorks left a comment

Choose a reason for hiding this comment

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

Looks good to me! I'll give this a test sometime in the next day or so and pull the merge trigger.

Copy link
Member

@Web-eWorks Web-eWorks left a comment

Choose a reason for hiding this comment

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

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

screenshot-20231008-123501

Here we are with tooltips

Copy link
Member

@Web-eWorks Web-eWorks left a comment

Choose a reason for hiding this comment

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

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.

data/pigui/modules/station-view/04-shipMarket.lua Outdated Show resolved Hide resolved
data/pigui/modules/station-view/04-shipMarket.lua Outdated Show resolved Hide resolved
data/pigui/modules/station-view/04-shipMarket.lua Outdated Show resolved Hide resolved
* 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
Copy link
Member

@Web-eWorks Web-eWorks left a comment

Choose a reason for hiding this comment

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

Looks good to me! You can manually clean up the git history, or I can squash-merge all the history into a single commit.

@Web-eWorks Web-eWorks merged commit c50b92d into pioneerspacesim:master Oct 8, 2023
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.

3 participants