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

Rectify incorrect densities of steel resource items #63365

Merged
merged 6 commits into from
Mar 14, 2023

Conversation

cake-pie
Copy link
Contributor

@cake-pie cake-pie commented Jan 31, 2023

Summary

Bugfixes "Rectify incorrect densities of steel resource items"

Purpose of change

Resolves #62928

  • rectify errors introduced in 8ccc9d9, 68052db and propagated by other subsequent commits.
  • reordering and reorganization of data/json/items/resources/metal.json so that related entries (material or form factor) are clustered together
  • move some misplaced items out of the file, to more appropriate locations
  • adopt copy-from for the many steel type variants to reduce redundancy

Describe the solution

Cross reference known good version of the file from before the regression.

Fix impacted budget steel smelting recipes per discussion.

Oven components fanblade_metal and heavy_wire_rack moved to appliance_parts.json
Door hinge hinge moved to home_improvement.json

Describe alternatives you've considered

Simply revert the commits responsible for the regression, but it's already been a few months and there have been other intervening commits.

Testing

Check in game that:

  • the item stats are sane
  • the budget steel smelting recipe inputs/outputs are sane

Additional context and future work

Part of #62936 which is being reorganized and re-done as it was starting to get out of hand.
Bits of that PR will continue to be split off as smaller, more manageable chunks and dealt with separately.

It was originally intended in #62936 to take advantage of the change from unitless weights/volumes to improve the accuracy of some of these stats which in the past had to be rounded off, leading to pretty crude approximations in some cases. That is now being deferred as a non-critical enhancement.

For instance, based on preexisting comments, we know that steel was intended to be 7.6g/cm³, but in practice it is 8.0 due to rounding errors (in the age of unitless stats). That can be rectified by adjusting either mass or volume; it seems preferable to leave mass untouched but further discussion may be warranted.

Note that steel ingots stats currently don't line up with the general density of other smaller steel resource items, as they appear to have used 7.859g/cm³. Leaving it alone for now as these are uncommon: they don't spawn in world, only player-craftable for bulk-storage and handling convenience. In any case, resolution of this discrepancy will depend on how steel density overall gets adjusted.

@github-actions github-actions bot added [JSON] Changes (can be) made in JSON Appliance/Power Grid Anything to do with appliances and power grid <Bugfix> This is a fix for a bug (or closes open issue) json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions Code: Tests Measurement, self-control, statistics, balancing. labels Jan 31, 2023
@cake-pie
Copy link
Contributor Author

cake-pie commented Jan 31, 2023

@Drew4484 I hope 2533c97 is satisfactory

@cake-pie
Copy link
Contributor Author

cake-pie commented Feb 1, 2023

Unrelated test failure likely originating from #62604

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Feb 5, 2023
@cake-pie
Copy link
Contributor Author

cake-pie commented Mar 2, 2023

Rebased to resolve conflicts.

@bombasticSlacks
Copy link
Contributor

Alright, so you need to move back hinges, blades, and wire (as well as any other moved items)

I understand messy organization can be frustrating, but moving the items means we lose all the history of changes to the entries which is a hard no for the project.

The same goes for grouping the items, moving them around messes with history (and also makes this PR very annoying to review). The reorganizing within the file causes the same issues.

doing either of these things the code will be rejected because we lose the history of the items and it breaks all other concurrent work.

Again sorry I know this must be frustrating and I wish I had noticed the issue sooner.

Copy link
Contributor

@bombasticSlacks bombasticSlacks left a comment

Choose a reason for hiding this comment

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

requesting changes so no one yolo merges this while it has these problems

data/json/items/resources/appliance_parts.json Outdated Show resolved Hide resolved
@cake-pie
Copy link
Contributor Author

cake-pie commented Mar 3, 2023

we lose all the history of changes to the entries

???

This is untrue -- keeping history of changes is the whole point of version control systems like git. I've previously traced change history in this project across moved/renamed files, files that were split up, and other instances of c++ code or json entries moved across different files; no problem whatsoever.

I don't understand what your complaint is, please elaborate,

very annoying to review

You do realize you can review changes commit-by-commit or for a subset of commits in the PR, right?

I make it a point to organize my work into commits that reflect coherent steps; in this case the sorting is in a separate commit of its own, which can be checked to ensure that entries were only moved as-is, without changes.

@bombasticSlacks
Copy link
Contributor

I spoke in simple terms because I didn't recognize your name, many people working on the project don't understand the nuances of GIT, so explaining things simply is important to get points across.

It looks like you've been doing a lot of work over the last while and you understand how software works, so I'll speak clearly:

The project position is that JSON organization is pointless data churn and that it isn't worth even the minor inconvenience of burying git history over reorganization commits. Someone else is just going to come along and fuck it up by putting stuff in between, so it just makes data gathering annoying for 0 long-term benefit.

You do realize you can review changes commit-by-commit or for a subset of commits in the PR, right?

I make it a point to organize my work into commits that reflect coherent steps; in this case the sorting is in a separate commit of its own, which can be checked to ensure that entries were only moved as-is, without changes.

150 PRs have been merged in the last 2 days. I would encourage you to have some perspective.

Hopefully, that makes the position of the developers clearer, you should incorporate that instruction into this PR and others, or they will not get merged.

@anothersimulacrum
Copy link
Member

The merits of moving entities around aside (though I think it ought be avoided).

in this case the sorting is in a separate commit of its own, which can be checked to ensure that entries were only moved as-is, without changes.

Human checking of this is rarely perfect, and effort intensive. The easier you make your PR to review, the faster it is to merge and less effort will be required of you.

@cake-pie
Copy link
Contributor Author

cake-pie commented Mar 14, 2023

unwanted commits dropped and overall rebased to resolve merge conflicts

@bombasticSlacks bombasticSlacks merged commit ba88121 into CleverRaven:master Mar 14, 2023
@cake-pie cake-pie deleted the mrid-steel branch March 14, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Appliance/Power Grid Anything to do with appliances and power grid astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) Code: Tests Measurement, self-control, statistics, balancing. [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Steel resource items have erroneous densities
3 participants