Deconstructing heavy machinery can give negative 1 billion 60L tanks, among other weirdness

[Technical-l]

Describe the bug

Deconstructing the heavy machinery object at the light industry gives a weird amount of items, including negative numbers and billions of items. It seems every time the game is restarted, the number of items you get from deconstruction changes.

Attach save file

Vinemont-trimmed.tar.gz

Steps to reproduce

1. Find heavy machinery
2. Attempt deconstruction

In this case, the heavy machinery was in a light industry building, the building without the safe.

Expected behavior

I expected that the deconstruction of heavy machinery would not give me negative 1 billion 60L tanks, because heavy machinery is usually not built with negative 1 billion 60L tanks.

Screenshots

Screenshot of the deconstruction on a fresh save, spawned with debug mode.

Screenshot of the deconstruction on the save i encountered this bug

Versions and configuration

• OS: Windows
  • OS Version: 10.0.22631.3880 (23H2)
• Game Version: b7ae537 [64-bit]
• Graphics Version: Tiles
• Game Language: System language []
• Mods loaded: [
  Dark Days Ahead [dda],
  Disable NPC Needs [no_npc_food],
  Portal Storms Ignore NPCs [personal_portal_storms],
  Slowdown Fungal Growth [no_fungal_growth],
  Bionic Slots [cbm_slots]
  ]

Additional context

No response

[achpile]

My wild guess would be #74440

Probably needs @Procyonae to look into this 🙂

[achpile]

Just another wild guess:

Item_modifier::count does not load properly if no "count" object exists: (since bug occurs only for instances with no count object)

    "deconstruct": {
      "items": [
        { "item": "wire", "count": [ 1, 3 ] },
        { "item": "pipe", "count": [ 1, 2 ] },
        { "item": "chain", "prob": 60 },
        { "item": "cu_pipe", "prob": 20 },
        { "item": "steel_lump", "count": [ 1, 2 ] },
        { "item": "hose", "count": 1 },
        { "item": "sheet_metal", "count": [ 1, 3 ] },
        { "item": "steel_chunk", "count": [ 1, 3 ] },
        { "item": "pipe_fittings", "count": [ 1, 3 ] },
        { "item": "bearing", "charges": [ 4, 12 ] },
        { "item": "frame", "prob": 60 },
        { "item": "motor", "prob": 30 },
        { "item": "metal_tank", "prob": 30 },
        { "item": "motor_large", "prob": 10 }
      ]
    },

This may lead to:

template<typename T>
bool load_min_max( std::pair<T, T> &pa, const JsonObject &obj, const std::string &name )
{
    bool result = false;
    if( obj.has_array( name ) ) {
        // An array means first is min, second entry is max. Both are mandatory.
        JsonArray arr = obj.get_array( name );
        result |= arr.read_next( pa.first );
        result |= arr.read_next( pa.second );
    } else {
        // Not an array, should be a single numeric value, which is set as min and max.
        result |= obj.read( name, pa.first );
        result |= obj.read( name, pa.second );
    }
    result |= obj.read( name + "-min", pa.first );
    result |= obj.read( name + "-max", pa.second );
    return result;
}

Where pa.first and pa.second should be set to 1 if no object named "count" provided.

But I also might be wrong

[achpile]

Okay. I found the issue!

item_group.cpp:491

std::map<const itype *, std::pair<int, int>> Single_item_creator::every_item_min_max() const
{
    switch( type ) {
        case S_ITEM: {
            const itype *i = item::find_type( itype_id( id ) );
            if( i->count_by_charges() ) {
                // TODO: Not technically perfect for only charge_min/charge max and for ammo/liquids but Item_modifier::modify()'s logic is gross and I'd rather not try to replicate it perfectly as is
                const int min_charges = modifier->charges.first == -1 ?
                                        i->charges_default() : modifier->charges.first;
                const int max_charges = modifier->charges.second == -1 ?
                                        i->charges_default() : modifier->charges.second;
                return { std::make_pair( i, std::make_pair( modifier->count.first * min_charges, modifier->count.second * max_charges ) ) };
            }
            return { std::make_pair( i, modifier->count ) }; <------ modifier here might be NULL
        }
        case S_ITEM_GROUP: {
            Item_spawn_data *isd = item_controller->get_group( item_group_id( id ) );
            if( isd != nullptr ) {
                return isd->every_item_min_max();
            }
            return {};
        }
        case S_NONE:
            return {};
    }
    // NOLINTNEXTLINE(misc-static-assert,cert-dcl03-c)
    cata_fatal( "Unexpected type" );
    return {};
}

Something like this should fix this:

diff --git a/src/item_group.cpp b/src/item_group.cpp
index bccb398..23f39b8 100644
--- a/src/item_group.cpp
+++ b/src/item_group.cpp
@@ -501,7 +501,11 @@ std::map<const itype *, std::pair<int, int>> Single_item_creator::every_item_min
                                         i->charges_default() : modifier->charges.second;
                 return { std::make_pair( i, std::make_pair( modifier->count.first * min_charges, modifier->count.second * max_charges ) ) };
             }
-            return { std::make_pair( i, modifier->count ) };
+
+            if ( modifier )
+                return { std::make_pair( i, modifier->count ) };
+            else
+                return { std::make_pair( i, std::make_pair( 1, 1 ) ) };
         }
         case S_ITEM_GROUP: {
             Item_spawn_data *isd = item_controller->get_group( item_group_id( id ) );

But I am not sure. So if true devs agree with this solution - I'll make a PR :)

If yes - then I'm not sure if something like this is appropriate...

return { std::make_pair( i, modifier ? modifier->count : std::make_pair( 1, 1 ) ) };

I mean which fits more to the main code style :D

Update:

Also if item has probability less than a 100 should it be pair(0, 1) ?

[Procyonae]

Probably needs @Procyonae to look into this 🙂

Whoops ye, nice catch ^_^