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

Ship category audit #2349

Conversation

DmitriTheDemon
Copy link
Contributor

@DmitriTheDemon DmitriTheDemon commented Oct 29, 2024

About the PR

Updated categories based on ship submission guidelines to be consistent. resolve #1962

Why / Balance

Categories for some ships were inconsistent with standards applied to other ships

Spreadsheet of current ship sizes and dimensions here

How to test

Go to each shipyard terminal
Verify ships show up in correct category
Buy and sell ships to ensure they do not break on load

Media

Ship Category - Exped Medium Ship Category - Frontier Medium Ship Category - Frontier Micro Ship Category - Frontier Small Ship Category - Scrap Medium

Requirements

Breaking changes

N/A

Changelog
🆑

  • tweak: Updated category (size) for pathfinder, hammer, legman, pioneer, and orange

Audit of ship categories based on dimensions from ship submission guidelines
Audit of ship categories based on dimensions from ship submission guidelines
@dvir001
Copy link
Contributor

dvir001 commented Oct 29, 2024

Based

@dvir001 dvir001 self-requested a review October 29, 2024 22:30
@github-actions github-actions bot added the Status: Needs Review This PR is awaiting reviews label Oct 29, 2024
@Tych0theSynth
Copy link
Contributor

May end up changing again in the future if the submission guidelines are updated

@DmitriTheDemon DmitriTheDemon marked this pull request as ready for review October 30, 2024 05:56
Copy link
Contributor

@arimah arimah left a comment

Choose a reason for hiding this comment

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

Please see my comment on the issue you're attempting to fix: #1962 (comment)

Tile count is not the only determiner. With this change, given the current rules, several ships actually get an incorrect size classification. Please update your PR according to the rules and we can get this merged. :)

@DmitriTheDemon
Copy link
Contributor Author

Please see my comment on the issue you're attempting to fix: #1962 (comment)

Tile count is not the only determiner. With this change, given the current rules, several ships actually get an incorrect size classification. Please update your PR according to the rules and we can get this merged. :)

In the spreadsheet I linked I manually gathered the dimensions (height and width) of the ships and compared that to the guidelines as well as evaluated the total footprint (area of length x width) compared with tile count in order to come up with a list of ships to adjust.

19 ships did not match based on the longest dimension (width/height).
7 of those were very narrow and well within the recommended tile count, so I did not adjust those.
7 are mail pod variants which are only 1 tile over at 9x5, so I believe it is correct to leave them in the micro category
5 remaining are changed in this pull request

The Hammer is currently a Medium, but it is only 1 tile over at 17x14 and I believe the 'feel' of the ship is that of a small vessel, so I added a change to small which is in line to how the mail pod is currently handled. If others do not agree with that suggestion I can revert it.

The Pioneer is 9x5 as the other mail pod variants, but it is the only one listed as a small. I changed this one to micro for consistency as even though it is not strictly speaking the same layout as a mail pod it does share the same dimensions.

If you have any specific alterations you would like to recommend for individual ships, please let me know and I will address them based on the consensus of the group.

@whatston3
Copy link
Contributor

Whenever a suitable criterion is found, it should be added to the shipyard tests.

@DmitriTheDemon
Copy link
Contributor Author

Whenever a suitable criterion is found, it should be added to the shipyard tests.

My understanding is that there was not an automated solution to determining the dimensions and tile counts of a given ship file when I was skimming through discord? That might involve parsing the mapping file which would be Fun

@DmitriTheDemon DmitriTheDemon requested a review from arimah November 1, 2024 17:30
@github-actions github-actions bot added the Merge Conflict This PR has conflicts that prevent merging label Nov 7, 2024
Copy link
Contributor

github-actions bot commented Nov 7, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@dvir001
Copy link
Contributor

dvir001 commented Nov 7, 2024

Why closed?

@DmitriTheDemon
Copy link
Contributor Author

Why closed?

I was told there is still ongoing discussion on how ships should be classified and this will require revisions in order to close, and the merge conflict will need to be addressed. Once I get feedback on what should be implemented I can correct and reopen.

@DmitriTheDemon DmitriTheDemon deleted the ship-category-audit branch December 4, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Map-Shuttle Map - Shuttle Merge Conflict This PR has conflicts that prevent merging No C# Status: Needs Review This PR is awaiting reviews YML
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple shuttles are of incorrect size groups
5 participants