Special attacks to optional

This requires a really gross reader because there's two interrelated
bits of data spread across two different variables, and the data
definition is complex, so needs some special handling for delete.

The loading of the array syntax is gross too.

But the bugs being fixed shows it is an improvement :)

Author: ehughsbaird

data/json/monsters/insect_spider.json

@@ -2370,7 +2370,7 @@
     "weakpoints": [ { "id": "abdomen", "name": "the abdomen", "coverage": 30 } ],
     "vision_night": 8,
     "zombify_into": "mon_meat_cocoon_tiny",
-    "delete": { "weakpoints": [ { "id": "stinger" } ], "flags": [ "FLIES" ], "special_attacks": [ "DERMATIK" ] }
+    "delete": { "weakpoints": [ { "id": "stinger" } ], "flags": [ "FLIES" ], "special_attacks": [ "dermatik_impale" ] }
   },
   {
     "id": "mon_dermatik_incubator_deer",

data/json/monsters/zed_nomex.json

@@ -32,7 +32,7 @@
       "families": [ "prof_wp_syn_armored", "prof_wp_nat_armored" ],
       "flags": [ "ACIDPROOF", "FIREPROOF" ]
     },
-    "delete": { "special_attacks": [ "bite_humanoid" ] }
+    "delete": { "special_attacks": [ "bite" ] }
   },
   {
     "id": "mon_zombie_fireproof_grappler",

data/mods/Xedra_Evolved/monsters/zombies.json

@@ -96,7 +96,7 @@
       "corpse_type": "NO_CORPSE"
     },
     "emit_fields": [ { "emit_id": "xedra_monochrome_boomer_frozen_time", "delay": "1 s" } ],
-    "delete": { "special_attacks": [ "BOOMER" ] },
+    "delete": { "special_attacks": [ "boomer_bile" ] },
     "extend": { "flags": [ "STABILIZED_TIMELINE" ] },
     "upgrades": false
   }

src/mattack_actors.cpp

@@ -93,6 +93,17 @@ static const skill_id skill_throw( "throw" );
 static const trait_id trait_TOXICFLESH( "TOXICFLESH" );
 static const trait_id trait_VAMPIRE( "VAMPIRE" );
 
+bool invalid_mattack_actor::call( monster &m ) const
+{
+    debugmsg( "%s has invalid mattack actor definition!", m.type->id.str() );
+    return false;
+}
+
+std::unique_ptr<mattack_actor> invalid_mattack_actor::clone() const
+{
+    return std::make_unique<invalid_mattack_actor>( *this );
+}
+
 void leap_actor::load_internal( const JsonObject &obj, const std::string & )
 {
     // Mandatory:

src/mattack_actors.h

@@ -22,6 +22,13 @@ class Creature;
 class JsonObject;
 class monster;
 
+class invalid_mattack_actor : public mattack_actor
+{
+        bool call( monster & ) const override;
+        std::unique_ptr<mattack_actor> clone() const override;
+        void load_internal( const JsonObject &, const std::string & ) override {}
+};
+
 class leap_actor : public mattack_actor
 {
     public:

src/mattack_common.h

@@ -44,7 +44,7 @@ class mattack_actor
 struct mtype_special_attack {
     protected:
         // TODO: Remove friend
-        friend struct mtype;
+        friend struct special_attacks_reader;
         cata::clone_ptr<mattack_actor> actor;
 
     public:

src/monstergenerator.cpp

@@ -762,6 +762,68 @@ void revive_type::deserialize( const JsonObject &jo )
     }
 }
 
+// this is a really gross reader - special_attacks and special_attacks_names really need to be one struct
+// Then this gets a lot less gross and a lot more safe!
+struct special_attacks_reader : generic_typed_reader<special_attacks_reader> {
+    friend class MonsterGenerator;
+    // For ordering purposes, we want to also grab the names when we load a special attack
+    std::vector<std::string> &names;
+
+    // special attacks load with src
+    std::string_view src;
+    mtype_id id;
+
+    special_attacks_reader( std::vector<std::string> &name_vec, std::string_view _src,
+                            mtype_id mtype ) : names( name_vec ), src( _src ), id( mtype ) {}
+
+    void report_double_def( const std::string &name, const JsonValue &jv ) const {
+        if( std::find( names.begin(), names.end(), name ) != names.end() ) {
+            jv.throw_error(
+                string_format( "%s specifies more than one attack of (sub)type %s, ignoring all but the last.  Add different `id`s to each attack of this type to prevent this.",
+                               id.c_str(), name ) );
+        }
+    }
+
+    std::pair<std::string, mtype_special_attack> get_next( const JsonValue &jv ) const {
+        MonsterGenerator &gen = MonsterGenerator::generator();
+        // only for delete
+        if( jv.test_string() ) {
+            std::string name = jv.get_string();
+            auto it = std::find( names.begin(), names.end(), name );
+            if( it == names.end() ) {
+                jv.throw_error(
+                    string_format( "Invalid special attack format or does not exist to delete for %s on %s",
+                                   name, id.str() ) );
+            }
+            names.erase( it );
+            return std::make_pair( name, mtype_special_attack( std::make_unique<invalid_mattack_actor>() ) );
+        }
+        if( jv.test_object() ) {
+            mtype_special_attack new_attack = gen.create_actor( jv.get_object(), std::string( src ) );
+            report_double_def( new_attack->id, jv );
+            names.push_back( new_attack->id );
+            return std::make_pair( new_attack->id, new_attack );
+        }
+        JsonArray inner = jv.get_array();
+        std::string name = inner.get_string( 0 );
+        const auto iter = gen.attack_map.find( name );
+        if( iter == gen.attack_map.end() ) {
+            inner.throw_error( "Invalid special_attacks" );
+        }
+        report_double_def( name, jv );
+
+        mtype_special_attack new_attack = mtype_special_attack( iter->second );
+        if( inner.has_array( 1 ) ) {
+            new_attack.actor->cooldown.min = get_dbl_or_var_part( inner.get_array( 1 )[0] );
+            new_attack.actor->cooldown.max = get_dbl_or_var_part( inner.get_array( 1 )[1] );
+        } else {
+            new_attack.actor->cooldown.min = get_dbl_or_var_part( inner[1] );
+        }
+        names.push_back( name );
+        return std::make_pair( name, new_attack );
+    }
+};
+
 void mtype::load( const JsonObject &jo, const std::string &src )
 {
     MonsterGenerator &gen = MonsterGenerator::generator();
@@ -958,24 +1020,18 @@ void mtype::load( const JsonObject &jo, const std::string &src )
         def_chance = 0;
     }
 
-    if( !was_loaded || jo.has_member( "special_attacks" ) ) {
-        special_attacks.clear();
+    // this is really gross, and special_attacks + special_attacks_names should be combined into one struct
+    if( jo.has_member( "special_attacks" ) ) {
         special_attacks_names.clear();
-        add_special_attacks( jo, "special_attacks", src );
-    } else {
-        // Note: special_attacks left as is, new attacks are added to it!
-        // Note: member name prefixes are compatible with those used by generic_typed_reader
-        if( jo.has_object( "extend" ) ) {
-            JsonObject tmp = jo.get_object( "extend" );
-            tmp.allow_omitted_members();
-            add_special_attacks( tmp, "special_attacks", src );
-        }
-        if( jo.has_object( "delete" ) ) {
-            JsonObject tmp = jo.get_object( "delete" );
-            tmp.allow_omitted_members();
-            remove_special_attacks( tmp, "special_attacks", src );
+    }
+    optional( jo, was_loaded, "special_attacks", special_attacks, special_attacks_reader{special_attacks_names, src, id} );
+    // to be extra safe
+    for( const std::pair<const std::string, mtype_special_attack> &atk : special_attacks ) {
+        if( dynamic_cast<const invalid_mattack_actor *>( atk.second.get() ) != nullptr ) {
+            jo.throw_error( string_format( "Invalid special attack format for %s", atk.first ) );
         }
     }
+
     optional( jo, was_loaded, "melee_training_cap", melee_training_cap, std::min( melee_skill + 2,
               MAX_SKILL ) );
     optional( jo, was_loaded, "chat_topics", chat_topics );
@@ -1326,6 +1382,8 @@ mtype_special_attack MonsterGenerator::create_actor( const JsonObject &obj,
         new_attack = std::make_unique<gun_actor>();
     } else if( attack_type == "spell" ) {
         new_attack = std::make_unique<mon_spellcasting_actor>();
+    } else if( attack_type == "invalid" ) {
+        new_attack = std::make_unique<invalid_mattack_actor>();
     } else {
         obj.throw_error_at( "attack_type", "unknown monster attack" );
     }
@@ -1356,87 +1414,6 @@ void MonsterGenerator::load_monster_attack( const JsonObject &jo, const std::str
     add_attack( create_actor( jo, src ) );
 }
 
-void mtype::add_special_attack( const JsonObject &obj, const std::string &src )
-{
-    mtype_special_attack new_attack = MonsterGenerator::generator().create_actor( obj, src );
-
-    if( special_attacks.count( new_attack->id ) > 0 ) {
-        special_attacks.erase( new_attack->id );
-        const auto iter = std::find( special_attacks_names.begin(), special_attacks_names.end(),
-                                     new_attack->id );
-        if( iter != special_attacks_names.end() ) {
-            special_attacks_names.erase( iter );
-        }
-        debugmsg( "%s specifies more than one attack of (sub)type %s, ignoring all but the last.  Add different `id`s to each attack of this type to prevent this.",
-                  id.c_str(), new_attack->id.c_str() );
-    }
-
-    special_attacks.emplace( new_attack->id, new_attack );
-    special_attacks_names.push_back( new_attack->id );
-}
-
-void mtype::add_special_attack( const JsonArray &inner, std::string_view )
-{
-    MonsterGenerator &gen = MonsterGenerator::generator();
-    const std::string name = inner.get_string( 0 );
-    const auto iter = gen.attack_map.find( name );
-    if( iter == gen.attack_map.end() ) {
-        inner.throw_error( "Invalid special_attacks" );
-    }
-
-    if( special_attacks.count( name ) > 0 ) {
-        special_attacks.erase( name );
-        const auto iter = std::find( special_attacks_names.begin(), special_attacks_names.end(), name );
-        if( iter != special_attacks_names.end() ) {
-            special_attacks_names.erase( iter );
-        }
-        if( test_mode ) {
-            debugmsg( "%s specifies more than one attack of (sub)type %s, ignoring all but the last",
-                      id.c_str(), name );
-        }
-    }
-    mtype_special_attack new_attack = mtype_special_attack( iter->second );
-    if( inner.has_array( 1 ) ) {
-        new_attack.actor->cooldown.min = get_dbl_or_var_part( inner.get_array( 1 )[0] );
-        new_attack.actor->cooldown.max = get_dbl_or_var_part( inner.get_array( 1 )[1] );
-    } else {
-        new_attack.actor->cooldown.min = get_dbl_or_var_part( inner[1] );
-    }
-    special_attacks.emplace( name, new_attack );
-    special_attacks_names.push_back( name );
-}
-
-void mtype::add_special_attacks( const JsonObject &jo, std::string_view member,
-                                 const std::string &src )
-{
-
-    if( !jo.has_array( member ) ) {
-        return;
-    }
-
-    for( const JsonValue entry : jo.get_array( member ) ) {
-        if( entry.test_array() ) {
-            add_special_attack( entry.get_array(), src );
-        } else if( entry.test_object() ) {
-            add_special_attack( entry.get_object(), src );
-        } else {
-            entry.throw_error( "array element is neither array nor object." );
-        }
-    }
-}
-
-void mtype::remove_special_attacks( const JsonObject &jo, std::string_view member_name,
-                                    std::string_view )
-{
-    for( const std::string &name : jo.get_tags( member_name ) ) {
-        special_attacks.erase( name );
-        const auto iter = std::find( special_attacks_names.begin(), special_attacks_names.end(), name );
-        if( iter != special_attacks_names.end() ) {
-            special_attacks_names.erase( iter );
-        }
-    }
-}
-
 void MonsterGenerator::check_monster_definitions() const
 {
     for( const mtype &mon : mon_templates->get_all() ) {

src/monstergenerator.h

@@ -82,6 +82,7 @@ class MonsterGenerator
         const std::vector<mtype> &get_all_mtypes() const;
         mtype_id get_valid_hallucination() const;
         friend struct mtype;
+        friend struct special_attacks_reader;
         friend struct species_type;
         friend class mattack_actor;
 

src/mtype.h

@@ -606,16 +606,6 @@ struct mtype {
 
         // Historically located in monstergenerator.cpp
         void load( const JsonObject &jo, const std::string &src );
-
-    private:
-
-        void add_special_attacks( const JsonObject &jo, std::string_view member_name,
-                                  const std::string &src );
-        void remove_special_attacks( const JsonObject &jo, std::string_view member_name,
-                                     std::string_view src );
-
-        void add_special_attack( const JsonArray &inner, std::string_view src );
-        void add_special_attack( const JsonObject &obj, const std::string &src );
 };
 
 #endif // CATA_SRC_MTYPE_H