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

Density sanity check #54747

Merged
merged 17 commits into from
Apr 8, 2022
Merged

Density sanity check #54747

merged 17 commits into from
Apr 8, 2022

Conversation

drhead
Copy link
Contributor

@drhead drhead commented Jan 24, 2022

Summary

Infrastructure "Sanity checking for item density based on materials"

Purpose of change

I have noticed countless items in-game with quite unreasonable densities -- off the top of my head, refrigeration coils (10g/cm^3, but made of steel which is 8g/cm^3), duct tape (3.2g/cm^3, unreasonable for plastic), felt sheets (literally twice as dense as iridium). With the amount of items in the game, I don't think it is reasonable to simply fix unrealistic densities as they are discovered, and I believe an automated test for item density would help identify and fix all existing items and avoid this in the future.

Describe the solution

I am adding an automated sanity check for item density to the test suite, in order to make sure that items aren't denser than something made of that material should be. I would also like it if densities in the materials json could be updated with real-world densities in order to make it easier to do this (this was brought up in #53908 but it was suggested that the density field is removed since it is not really used for much -- the main reason I am opening a WIP pull request right now is so that this doesn't happen in the meantime).

I do wish to hear some feedback and discussion as to what certain material densities should be. Of what I've gone over so far:

  • For things like copper and aluminum there's not much discussion to be had since these have well-defined real world values.
  • Things like steel have a narrow range, but the game currently includes 8 varieties of steel plus iron, I think it's fine to leave all of those at 8 g/cm^3 for simplicity but specific ones could be defined with precise values for a specific alloy.
  • Bronze and brass covers a broader range of alloys with varying densities -- I've chosen 8.8 g/cm^3 as a median value, but some alloys apparently go up to 9.25 g/cm^3.
  • Ceramic covers a very broad range of materials. The densest material commonly used for armor plates is titanium diboride, with a density of 4.5 g/cm^3, and I think this covers most in-game applications for ceramic (keeping in mind that this check covers upper limits only)

Describe alternatives you've considered

Any amount of unreasonable item weight/volume values could be found manually, but I don't think this is desirable in the long term. Plus having settled values for density is helpful for contributors.

The current branch I have this PR on has a draft for a hardcoded list of values in the testing function itself, I don't like the idea of doing this and would much rather use JSON values for it as long as density can be a float value without breaking anything.

With how volume is treated for most items, very few if any items should be close to the densities of their main material -- even sheet metal has a density of about 6g/cm^3, which is only 75% of the actual density of steel. I think it might be helpful to have some method of generating a report that shows items that are close so that they can be looked at, but I don't think that should be something that is a fail condition on its own.

Testing

That's what this does!

It appears to be correctly identifying a great number of items, however I believe some of the known bad items need to be fixed before I can effectively narrow down any problems that may exist.

@github-actions github-actions bot added the json-styled JSON lint passed, label assigned by github actions label Jan 24, 2022
@PatrikLundell
Copy link
Contributor

I'm not sure a hard upper limit of the main material density is good, as I can imagine cases where something is mainly made out of some material, but has a small amount of something heavy in it (lead tipped blunt heavy crossbow bolt primarily made out of wood?). Thus, if you introduce automatic testing, you should also introduce a means to override test rejections.

@drhead
Copy link
Contributor Author

drhead commented Jan 25, 2022

I'm not sure a hard upper limit of the main material density is good, as I can imagine cases where something is mainly made out of some material, but has a small amount of something heavy in it (lead tipped blunt heavy crossbow bolt primarily made out of wood?). Thus, if you introduce automatic testing, you should also introduce a means to override test rejections.

The assumption I was planning on making was that materials in an object are sorted by the percent of volume they are present in. If someone comes by and declares that materials are actually listed in order of mass, that can be adjusted for as well. Or if it's officially not sorted by that it could check against only the densest material.

So a lead tipped crossbow bolt would be made of "Wood, Lead" and I would consider it to be made of at least 50% wood. It will pass if it is not heavier than a rod made of 50% wood and 50% lead of the specified volume. If the mass of the bolt is high enough to suggest that it is more than 50% lead by volume, I would say that should fail the test. So a small amount of something heavy won't be a problem at all, as long as it is actually a small amount.

If it was the other way around, "Lead, Wood", and thus assumed to be made of 50% lead, it would pass unless the bolts were really extremely heavy -- the only thing that would trip it is if it was heavier than a rod of pure lead of the same volume. Since lead is heavier, the model for failure would be effectively a lead rod with an infinitesimally small amount of wood.

@github-actions github-actions bot removed the json-styled JSON lint passed, label assigned by github actions label Jan 25, 2022
@github-actions github-actions bot added the json-styled JSON lint passed, label assigned by github actions label Jan 25, 2022
@bombasticSlacks
Copy link
Contributor

I'm not sure a hard upper limit of the main material density is good, as I can imagine cases where something is mainly made out of some material, but has a small amount of something heavy in it (lead tipped blunt heavy crossbow bolt primarily made out of wood?). Thus, if you introduce automatic testing, you should also introduce a means to override test rejections.

The assumption I was planning on making was that materials in an object are sorted by the percent of volume they are present in. If someone comes by and declares that materials are actually listed in order of mass, that can be adjusted for as well. Or if it's officially not sorted by that it could check against only the densest material.

So a lead tipped crossbow bolt would be made of "Wood, Lead" and I would consider it to be made of at least 50% wood. It will pass if it is not heavier than a rod made of 50% wood and 50% lead of the specified volume. If the mass of the bolt is high enough to suggest that it is more than 50% lead by volume, I would say that should fail the test. So a small amount of something heavy won't be a problem at all, as long as it is actually a small amount.

If it was the other way around, "Lead, Wood", and thus assumed to be made of 50% lead, it would pass unless the bolts were really extremely heavy -- the only thing that would trip it is if it was heavier than a rod of pure lead of the same volume. Since lead is heavier, the model for failure would be effectively a lead rod with an infinitesimally small amount of wood.

I don't know if you want info that will make this more complex but you can actually specify how much of an item is made of each material. Like 9 parts steel 1 parts aluminum. It was a fairly recent feature.

@drhead
Copy link
Contributor Author

drhead commented Jan 26, 2022

I don't know if you want info that will make this more complex but you can actually specify how much of an item is made of each material. Like 9 parts steel 1 parts aluminum. It was a fairly recent feature.

I did actually notice that while implementing it! I did implement it with that, and it turned out to be much simpler. It'll likely result in more items needing correction, though.

@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label Jan 26, 2022
@drhead drhead changed the title [WIP] Density sanity check Density sanity check Jan 26, 2022
@drhead drhead marked this pull request as ready for review January 26, 2022 09:16
@drhead
Copy link
Contributor Author

drhead commented Jan 26, 2022

Marking this as ready for review. The sanity check appears to work now, and has identified 822 items as having bad densities. Many of these are close to the density specified for materials, but instead of adding a margin of error these should probably be corrected anyways, since most items have densities far lower than being perfect. I have set all organic matter to a density of 1.1g/cm3, I would be willing to tweak that if it knocks off a few hundred items.

I don't think I can reasonably update 822 items on my own, and this will obviously fail tests until all 822 objects are updated. I would appreciate any help offered.

@Zireael07
Copy link
Contributor

How many wrong items are left if you DO add a (reasonable) margin of error? Just wondering how many items in those 842 are obviously wrong/huge outliers, and how many are reasonable...

@PatrikLundell
Copy link
Contributor

What about making an issue that lists all the recipes you need to have updated? If possible grouped so any offered help could bite off reasonably sized chunks of it.

I consider it a very bad option to accept a new test PR that immediately is going to flag each and every PR made as failing (it's been done in the past, and that was not good then either). Once the test doesn't fail on the existing code base it could be added,

@drhead
Copy link
Contributor Author

drhead commented Jan 26, 2022

How many wrong items are left if you DO add a (reasonable) margin of error? Just wondering how many items in those 842 are obviously wrong/huge outliers, and how many are reasonable...

The majority of close values seem to be what appears to be a minority of organic items which have densities from just above 1.1 to just above 1.3 (I set all organics to 1.1, which IS the margin of error) -- I might just up flesh materials to 1.2 because of how many things are at 1.184. There are some like beer that appear to be legitimately within a reasonable margin of error. Liquids are one of the very few things I'd really say should be represented as exactly actual density, I might try adding that once the list is narrowed down but I know I definitely won't exempt liquids because I see stuff like burdock wine being almost as dense as steel.

Most of them, I'd say, are clear outliers, and as I said before, if sheet metal is stored at about 75% efficiency, few other things should be close to actual material density.

What about making an issue that lists all the recipes you need to have updated? If possible grouped so any offered help could bite off reasonably sized chunks of it.

Sounds like a plan.

@Zireael07
Copy link
Contributor

For reference, the issue is at #54811

@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not 'bump' or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered.

@stale stale bot added the stale Closed for lack of activity, but still valid. label Mar 2, 2022
@stale stale bot removed the stale Closed for lack of activity, but still valid. label Mar 12, 2022
@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` [JSON] Changes (can be) made in JSON Code: Tests Measurement, self-control, statistics, balancing. labels Mar 12, 2022
@Maleclypse
Copy link
Member

Maleclypse commented Mar 12, 2022

@bombasticSlacks when I was fixing merge conflicts on this I tried to defer to what it looked like you were using for your breathability c++. If that fails I'm not really sure what to do to get this into merge shape since it looks like this person did a lot of hard work to make the density numbers correct.

edit: maybe they needed to be floats instead of ints but I imagine that would have larger implications than I can realize.

@bombasticSlacks
Copy link
Contributor

diff --git a/src/material.cpp b/src/material.cpp
index 97425a3ba2..d8f99557ba 100644
--- a/src/material.cpp
+++ b/src/material.cpp
@@ -250,7 +250,7 @@ float material_type::freeze_point() const
     return _freeze_point;
 }
 
-int material_type::density() const
+float material_type::density() const
 {
     return _density;
 }
diff --git a/src/material.h b/src/material.h
index cc921c08e4..d93d8f367a 100644
--- a/src/material.h
+++ b/src/material.h
@@ -86,7 +86,7 @@ class material_type
         float _fire_resist = 0.0f;
         float _bullet_resist = 0.0f;
         int _chip_resist = 0;                         // Resistance to physical damage of the item itself
-        int _density = 1;                             // relative to "powder", which is 1
+        float _density = 1.0f;                             // relative to "powder", which is 1
         // ability of a fabric to allow moisture vapor to be transmitted through the material
         breathability_rating _breathability = breathability_rating::IMPERMEABLE;
         // How resistant this material is to wind as a percentage - 0 to 100
@@ -144,7 +144,7 @@ class material_type
         float specific_heat_solid() const;
         float latent_heat() const;
         float freeze_point() const;
-        int density() const;
+        float density() const;
         // converts from the breathability enum to a fixed integer value from 0-100
         int breathability() const;
         cata::optional<int> wind_resist() const;
diff --git a/tests/item_test.cpp b/tests/item_test.cpp
index 78f4c1235d..e12ab7650a 100644
--- a/tests/item_test.cpp
+++ b/tests/item_test.cpp
@@ -716,11 +716,12 @@ static void assert_maximum_density_for_material( const item &target )
             max_density += m.first.obj().density() * m.second / target.type->mat_portion_total;
         }
         INFO( target.type_name() );
+        // this test will NOT pass right now so for now check but allow failing
         CHECK( item_density <= max_density );
     }
 }
 
-TEST_CASE( "item material density sanity check", "[item]" )
+TEST_CASE( "item_material_density_sanity_check", "[item][!mayfail]" )
 {
     for( const itype *type : item_controller->all() ) {
         const item sample( type, calendar::turn_zero, item::solitary_tag{} );

tests/item_test.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the astyled astyled PR, label is assigned by github actions label Mar 12, 2022
@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions and removed BasicBuildPassed This PR builds correctly, label assigned by github actions labels Mar 12, 2022
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Mar 28, 2022
@kevingranade kevingranade merged commit e48cccf into CleverRaven:master Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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: Tests Measurement, self-control, statistics, balancing. [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions
Projects
Development

Successfully merging this pull request may close these issues.

7 participants