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

Change all instances of "volume" in JSONs to be a metric string #33367

Merged
merged 18 commits into from
Aug 21, 2019

Conversation

ampersand55
Copy link
Contributor

Summary

SUMMARY: Infrastructure "Change all instances of 'volume' in JSONs to be a metric string"

Purpose of change

Metric units in string format ("L" or "ml") is preferred to units of 250ml.

See this exchange #33347 (comment) I had with @anothersimulacrum.

Describe the solution

Made a simple Node.js script that walks the data\json directory and replaces all instances of volume, max_volume and min_volume using integers with a metric string using a regexp. See:

https://github.com/ampersand55/CddaTools/blob/master/cddaUpdateJsonVolume.js

It's not pretty, but it works for now. Some problems:

  • Identifies and skips sound volume of a musical_instrument purely based on formatting which is not a robust way of doing it.
  • Does only change volume that's on a separate row, not inline-volume, e.g. { "volume": 2, "othervalue": "something"}. This is because that's used for proportional volume changes.

Describe alternatives you've considered

Making a proper JSON parser that converts it to a js object so you can check the depth and name of parent keys.

Additional context

@ymber
Copy link
Member

ymber commented Aug 19, 2019

This is almost exactly what I wrote adjust_values.py for. It parses json so you might want to adapt it for whatever json change automation you want.

@ampersand55
Copy link
Contributor Author

This is almost exactly what I wrote adjust_values.py for. It parses json so you might want to adapt it for whatever json change automation you want.

That was mentioned, but I couldn't get it to work. Now I know it was because I was on python 3.6 which didn't support f-strings. I still have problems with the linter though.

@ymber
Copy link
Member

ymber commented Aug 19, 2019

Python 3.6 was the version that introduced fstrings. That script should work with 3.6. If you're on windows your problem with the part where it lints stuff is probably that the formatter executable name is different on windows and I don't know if what I used there is a valid command for the windows shell.

@ampersand55
Copy link
Contributor Author

Updating to 3.7 fixed the error anyway.

I have some trouble with the linting in adjust_values.py, it seems to be referencing ../format/json_formatter.cgi which does not exist.

Copy link
Member

@anothersimulacrum anothersimulacrum left a comment

Choose a reason for hiding this comment

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

I've reviewed the first 100 files, up to data/json/items/book/unarmed.json. I've noticed no discrepancies, but a large number of items with zero volume, which I initially suggested fixes for, but have decided is out of the scope of this PR, minus the few I found a good answer for.

data/json/items/armor.json Outdated Show resolved Hide resolved
data/json/items/armor.json Outdated Show resolved Hide resolved
data/json/items/armor.json Outdated Show resolved Hide resolved
data/json/items/armor.json Outdated Show resolved Hide resolved
ampersand55 and others added 4 commits August 19, 2019 20:11
Co-Authored-By: anothersimulacrum <[email protected]>
Co-Authored-By: anothersimulacrum <[email protected]>
Co-Authored-By: anothersimulacrum <[email protected]>
Co-Authored-By: anothersimulacrum <[email protected]>
@ampersand55
Copy link
Contributor Author

Should I just skip updating the lines with "volume": 0 and leave them as is?

@ZhilkinSerg ZhilkinSerg added [JSON] Changes (can be) made in JSON Items / Item Actions / Item Qualities Items and how they work and interact labels Aug 19, 2019
@anothersimulacrum
Copy link
Member

Yeah, there's a lot, and doing so would hold this up.
This will make it much easier to find them and fix them in the future.

@ampersand55
Copy link
Contributor Author

Someone should update doc/JSON_INFO.md with the use of integer units of 250ml being deprecated.

@ampersand55
Copy link
Contributor Author

I tested that metric units could be used with integral_volume using a gyroscopic stabilizer installed on an mp5.

@kevingranade kevingranade merged commit e706ab9 into CleverRaven:master Aug 21, 2019
@ZhilkinSerg
Copy link
Contributor

storage should also probably be updated.

@int-ua
Copy link
Contributor

int-ua commented Aug 22, 2019

I've just updated to the latest build and furniture that had > 1000L storage now has only 1000L. Display rack, bookshelf, dresser. Can you confirm?

@ZhilkinSerg
Copy link
Contributor

I can see that locker going up from 500 to 1000:

image

@ampersand55
Copy link
Contributor Author

ampersand55 commented Aug 23, 2019

I've just updated to the latest build and furniture that had > 1000L storage now has only 1000L. Display rack, bookshelf, dresser. Can you confirm?

Can confirm. Every furniture max_storage seem to be 1000 L, even mail boxes.

They have correct string metric units (e.g. f_bookcase went from max_volume: 8000 to max_volume: "2000 L"), but the game might not be able to handle max_volume in string metric units for furniture.

Here is a revert branch:

But I think making furniture able to handle metric units for max_volume would be a better solution.

@ampersand55
Copy link
Contributor Author

Btw, I have a working proper robust json parser now that can change change keys based on type, category, depth, parent nodes, file name etc. But it outputs raw json and I can't get the json linter working. I can make it use the web linter, but that's not practical when changing hundreds of files in bulk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Items / Item Actions / Item Qualities Items and how they work and interact [JSON] Changes (can be) made in JSON
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants