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.

Sign up for GitHub

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

Jump to bottom

Use microgram (μg) resolution mass for food vitamins #68988

Merged
merged 1 commit into from
Nov 7, 2023
Merged

Use microgram (μg) resolution mass for food vitamins #68988

merged 1 commit into from
Nov 7, 2023

Conversation

ehughsbaird
Copy link
Contributor

@ehughsbaird ehughsbaird commented Oct 29, 2023 •
edited
Loading

Summary

SUMMARY: None

Built on #68986. Please do not merge until that is merged.

Purpose of change

#68986
Vitamins are often present in very small quantities, so to specify vitamins by weight, more precision that 1mg is required for mass.

Describe the solution

Introduce and use a special higher-precision units type for vitamin mass, and plug it into the code to all the vitamin mass parsing code.

μg, ug, mcg are supported when representing mass strings in JSON.

Testing

Vitamins on protein drink, shake, and fortified shake are loaded correctly.

Additional context

Thanks for the assistance in coming to this solution.

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Oct 29, 2023
@GuardianDll
Copy link
Member

I recommend to add mcg to the possible units

@kevingranade
Copy link
Member

This is a singular, obscure use case that is establishing a ratio, changing the base mass for everything in the game goes too far.

All you need to do here is refrain from changing the default mass unit and explicitly construct a mass type with the desired unit inside the vitamin deserialization code and in the associated data structures.

@PatrikLundell
Copy link
Contributor

A nitpick: What you're actually talking about is "resolution", not "precision". Any resolution in the world can't save you from loss of precision from a sequence of unfortunate or poorly chosen computational steps, such as division between two very close values.
However, higher resolution allows you to provide more precision if it isn't squandered.

@ehughsbaird ehughsbaird changed the title [CR] Units: mass has microgram (μg) precision Use microgram (μg) resolution mass for food vitamins Oct 30, 2023
@github-actions github-actions bot added <Documentation> Design documents, internal info, guides and help. [JSON] Changes (can be) made in JSON Items: Food / Vitamins Comestibles and drinks Code: Tests Measurement, self-control, statistics, balancing. [Markdown] Markdown issues and PRs labels Oct 30, 2023
@github-actions
Copy link
Contributor

It appears you modified a .po or .pot file. These translation files are automatically generated, pushed to, and pulled from Transifex, and should not be modified otherwise. If these changes are intended, please add Translation file changes are intended to the PR body.

@github-actions github-actions bot added NPC / Factions NPCs, AI, Speech, Factions, Ownership Translation I18n Mods Issues related to mods or modding Crafting / Construction / Recipes Includes: Uncrafting / Disassembling Mutations / Traits / Professions/ Hobbies Mutations / Traits / Professions/ Hobbies Mods: Dinomod Anything to do with the Dinoclysm mod (DinoMod) Items: Armor / Clothing Armor and clothing Mods: Mind Over Matter labels Oct 30, 2023
@ehughsbaird
Copy link
Contributor Author

I made a little bit of a mess, but it should be cleaned up now. I've updated to follow Kevin's suggestion.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Oct 30, 2023
@Maleclypse
Copy link
Member

Looks like one of the builds has errors from this?

38s
Set cache size limit to 5.0 GB
+ ccache --show-stats --verbose
Summary:
  Cache directory:  /home/runner/.cache/ccache
  Primary config:   /home/runner/.config/ccache/ccache.conf
  Secondary config: /etc/ccache.conf
  Stats updated:    Mon Oct 30 19:00:21 2023
  Hits:                0 /    0
    Direct:            0 /    0
+ '[' 0 = 1 ']'
+ make -j 3 RELEASE=1 CCACHE=1 CROSS= LINTJSON=0 FRAMEWORK=1 UNIVERSAL_BINARY=1
    Preprocessed:      0 /    0
  Misses:              0
    Direct:            0
    Preprocessed:      0
Primary storage:
  Hits:                0 /    0
  Misses:              0
  Cache size (GB):  2.94 / 5.00 (58.80 %)
  Files:            3373
Cannot run an astyle check, your system either does not have astyle, or it is too old.
CCACHE_SLOPPINESS=pch_defines,time_macros,include_file_ctime,include_file_mtime ccache g++-9 -Isrc -isystem src/third-party -DRELEASE -DGIT_VERSION -DBACKTRACE -DLIBBACKTRACE -DLOCALIZE -Os -flto=jobserver -flto-odr-type-merging -Wodr -Werror -Wall -Wextra -Wformat-signedness -Wlogical-op -Wmissing-declarations -Wmissing-noreturn -Wnon-virtual-dtor -Wold-style-cast -Woverloaded-virtual -Wpedantic -Wsuggest-override -Wunused-macros -Wzero-as-null-pointer-constant -Wno-unknown-warning-option -Wno-dangling-reference -Wno-c++20-compat -Wredundant-decls  -g1  -fsigned-char  -std=c++17 -m64 -fuse-ld=gold -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600 -MMD -MP -Wno-error -c pch/main-pch.hpp -o pch/main-pch.hpp.gch
// NOLINT(cata-header-guard)
#define VERSION "18dded7"
mkdir -p data/mods/TEST_DATA/lang/mo/ru/LC_MESSAGES/
msgfmt -f -o data/mods/TEST_DATA/lang/mo/ru/LC_MESSAGES/TEST_DATA.mo data/mods/TEST_DATA/lang/po/ru.po
CCACHE_SLOPPINESS=pch_defines,time_macros,include_file_ctime,include_file_mtime ccache g++-9 -Isrc -isystem src/third-party -DRELEASE -DGIT_VERSION -DBACKTRACE -DLIBBACKTRACE -DLOCALIZE -Os -flto=jobserver -flto-odr-type-merging -Wodr -Werror -Wall -Wextra -Wformat-signedness -Wlogical-op -Wmissing-declarations -Wmissing-noreturn -Wnon-virtual-dtor -Wold-style-cast -Woverloaded-virtual -Wpedantic -Wsuggest-override -Wunused-macros -Wzero-as-null-pointer-constant -Wno-unknown-warning-option -Wno-dangling-reference -Wno-c++20-compat -Wredundant-decls  -g1  -fsigned-char  -std=c++17 -m64 -fuse-ld=gold -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600 -MMD -MP -Winvalid-pch -include pch/main-pch.hpp -c src/achievement.cpp -o obj/achievement.o
CCACHE_SLOPPINESS=pch_defines,time_macros,include_file_ctime,include_file_mtime ccache g++-9 -Isrc -isystem src/third-party -DRELEASE -DGIT_VERSION -DBACKTRACE -DLIBBACKTRACE -DLOCALIZE -Os -flto=jobserver -flto-odr-type-merging -Wodr -Werror -Wall -Wextra -Wformat-signedness -Wlogical-op -Wmissing-declarations -Wmissing-noreturn -Wnon-virtual-dtor -Wold-style-cast -Woverloaded-virtual -Wpedantic -Wsuggest-override -Wunused-macros -Wzero-as-null-pointer-constant -Wno-unknown-warning-option -Wno-dangling-reference -Wno-c++20-compat -Wredundant-decls  -g1  -fsigned-char  -std=c++17 -m64 -fuse-ld=gold -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600 -MMD -MP -Winvalid-pch -include pch/main-pch.hpp -c src/action.cpp -o obj/action.o
CCACHE_SLOPPINESS=pch_defines,time_macros,include_file_ctime,include_file_mtime ccache g++-9 -Isrc -isystem src/third-party -DRELEASE -DGIT_VERSION -DBACKTRACE -DLIBBACKTRACE -DLOCALIZE -Os -flto=jobserver -flto-odr-type-merging -Wodr -Werror -Wall -Wextra -Wformat-signedness -Wlogical-op -Wmissing-declarations -Wmissing-noreturn -Wnon-virtual-dtor -Wold-style-cast -Woverloaded-virtual -Wpedantic -Wsuggest-override -Wunused-macros -Wzero-as-null-pointer-constant -Wno-unknown-warning-option -Wno-dangling-reference -Wno-c++20-compat -Wredundant-decls  -g1  -fsigned-char  -std=c++17 -m64 -fuse-ld=gold -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600 -MMD -MP -Winvalid-pch -include pch/main-pch.hpp -c src/active_item_cache.cpp -o obj/active_item_cache.o
CCACHE_SLOPPINESS=pch_defines,time_macros,include_file_ctime,include_file_mtime ccache g++-9 -Isrc -isystem src/third-party -DRELEASE -DGIT_VERSION -DBACKTRACE -DLIBBACKTRACE -DLOCALIZE -Os -flto=jobserver -flto-odr-type-merging -Wodr -Werror -Wall -Wextra -Wformat-signedness -Wlogical-op -Wmissing-declarations -Wmissing-noreturn -Wnon-virtual-dtor -Wold-style-cast -Woverloaded-virtual -Wpedantic -Wsuggest-override -Wunused-macros -Wzero-as-null-pointer-constant -Wno-unknown-warning-option -Wno-dangling-reference -Wno-c++20-compat -Wredundant-decls  -g1  -fsigned-char  -std=c++17 -m64 -fuse-ld=gold -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600 -MMD -MP -Winvalid-pch -include pch/main-pch.hpp -c src/activity_actor.cpp -o obj/activity_actor.o
CCACHE_SLOPPINESS=pch_defines,time_macros,include_file_ctime,include_file_mtime ccache g++-9 -Isrc -isystem src/third-party -DRELEASE -DGIT_VERSION -DBACKTRACE -DLIBBACKTRACE -DLOCALIZE -Os -flto=jobserver -flto-odr-type-merging -Wodr -Werror -Wall -Wextra -Wformat-signedness -Wlogical-op -Wmissing-declarations -Wmissing-noreturn -Wnon-virtual-dtor -Wold-style-cast -Woverloaded-virtual -Wpedantic -Wsuggest-override -Wunused-macros -Wzero-as-null-pointer-constant -Wno-unknown-warning-option -Wno-dangling-reference -Wno-c++20-compat -Wredundant-decls  -g1  -fsigned-char  -std=c++17 -m64 -fuse-ld=gold -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600 -MMD -MP -Winvalid-pch -include pch/main-pch.hpp -c src/activity_handlers.cpp -o obj/activity_handlers.o
In file included from src/character.h:[53](https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/6695033476/job/18189699067?pr=68988#step:14:54),
                 from src/avatar.h:15,
                 from src/action.cpp:13:
Error: src/stomach.h:32:2: error: extra ‘;’ [-Werror=pedantic]
   32 | }; // namespace vitamin_units
      |  ^
cc1plus: error: unrecognized command line option ‘-Wno-c++20-compat’ [-Werror]
cc1plus: error: unrecognized command line option ‘-Wno-dangling-reference’ [-Werror]
cc1plus: error: unrecognized command line option ‘-Wno-unknown-warning-option’ [-Werror]
cc1plus: all warnings being treated as errors
make: *** [Makefile:1001: obj/action.o] Error 1
make: *** Waiting for unfinished jobs....
In file included from src/character.h:53,
                 from src/activity_actor_definitions.h:15,
                 from src/advanced_inv.h:8,
                 from src/activity_handlers.cpp:20:
Error: src/stomach.h:32:2: error: extra ‘;’ [-Werror=pedantic]
   32 | }; // namespace vitamin_units
      |  ^
cc1plus: error: unrecognized command line option ‘-Wno-c++20-compat’ [-Werror]
cc1plus: error: unrecognized command line option ‘-Wno-dangling-reference’ [-Werror]
cc1plus: error: unrecognized command line option ‘-Wno-unknown-warning-option’ [-Werror]
cc1plus: all warnings being treated as errors
make: *** [Makefile:[100](https://github.com/CleverRaven/Cataclysm-DDA/actions/runs/6695033476/job/18189699067?pr=68988#step:14:101)1: obj/activity_handlers.o] Error 1
In file included from src/character.h:53,
                 from src/activity_actor_definitions.h:15,
                 from src/activity_actor.cpp:20:
Error: src/stomach.h:32:2: error: extra ‘;’ [-Werror=pedantic]
   32 | }; // namespace vitamin_units
      |  ^
cc1plus: error: unrecognized command line option ‘-Wno-c++20-compat’ [-Werror]
cc1plus: error: unrecognized command line option ‘-Wno-dangling-reference’ [-Werror]
cc1plus: error: unrecognized command line option ‘-Wno-unknown-warning-option’ [-Werror]
cc1plus: all warnings being treated as errors
make: *** [Makefile:1001: obj/activity_actor.o] Error 1
Error: Process completed with exit code 2.

@ehughsbaird
Copy link
Contributor Author

Yes. This isn't ready for merge yet, I was going to wait until #68986 is merged and fix it then.

@ehughsbaird
Use higher precision mass units for vitamins
2f9bc68 
Vitamins are often present in very small quantities, so to specify
vitamins by weight, more precision that 1mg is required for mass.

Introduce and use a special higher-precision units type for vitamin
mass, and plug it into the code to all the vitamin mass parsing code.

'μg', 'ug', 'mcg' are supported when representing mass strings in JSON.
@Maleclypse Maleclypse merged commit 7fecb62 into CleverRaven:master Nov 7, 2023
25 of 26 checks passed
@ehughsbaird ehughsbaird deleted the units-weight-more branch November 7, 2023 00:40
This was referenced Nov 10, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Code: Tests Measurement, self-control, statistics, balancing. Crafting / Construction / Recipes Includes: Uncrafting / Disassembling <Documentation> Design documents, internal info, guides and help. Items: Armor / Clothing Armor and clothing Items: Food / Vitamins Comestibles and drinks [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions [Markdown] Markdown issues and PRs Mods: Dinomod Anything to do with the Dinoclysm mod (DinoMod) Mods: Mind Over Matter Mods Issues related to mods or modding Mutations / Traits / Professions/ Hobbies Mutations / Traits / Professions/ Hobbies NPC / Factions NPCs, AI, Speech, Factions, Ownership Translation I18n
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ehughsbaird @GuardianDll @kevingranade @PatrikLundell @Maleclypse