Merge pull request #82309 from sparr/dedupe_effect_intensity_clamp

Refactor effect parameter updating

Author: Maleclypse

src/creature.cpp

@@ -1830,14 +1830,6 @@ void Creature::add_effect( const effect_source &source, const efftype_id &eff_id
             const int prev_int = e.get_intensity();
             // If we do, mod the duration, factoring in the mod value
             e.mod_duration( dur * e.get_dur_add_perc() / 100 );
-            // Limit to max duration
-            if( e.get_duration() > e.get_max_duration() ) {
-                e.set_duration( e.get_max_duration() );
-            }
-            // Adding a permanent effect makes it permanent
-            if( e.is_permanent() ) {
-                e.pause_effect();
-            }
             // int_dur_factor overrides all other intensity settings
             // ...but it's handled in set_duration, so explicitly do nothing here
             if( e.get_int_dur_factor() > 0_turns ) {
@@ -1848,14 +1840,8 @@ void Creature::add_effect( const effect_source &source, const efftype_id &eff_id
             } else if( e.get_int_add_val() != 0 ) {
                 e.mod_intensity( e.get_int_add_val(), is_avatar() );
             }
-
-            // Bound intensity by [1, max intensity]
-            if( e.get_intensity() < 1 ) {
-                add_msg_debug( debugmode::DF_CREATURE, "Bad intensity, ID: %s", e.get_id().c_str() );
-                e.set_intensity( 1 );
-            } else if( e.get_intensity() > e.get_max_intensity() ) {
-                e.set_intensity( e.get_max_intensity() );
-            }
+            // Bound new effect intensity by [1, max intensity]
+            e.clamp_intensity();
             if( e.get_intensity() != prev_int ) {
                 on_effect_int_change( eff_id, e.get_intensity(), bp );
             }
@@ -1867,23 +1853,7 @@ void Creature::add_effect( const effect_source &source, const efftype_id &eff_id
 
         // Now we can make the new effect for application
         effect e( effect_source( source ), &type, dur, bp.id(), permanent, intensity, calendar::turn );
-        // Bound to max duration
-        if( e.get_duration() > e.get_max_duration() ) {
-            e.set_duration( e.get_max_duration() );
-        }
 
-        // Force intensity if it is duration based
-        if( e.get_int_dur_factor() != 0_turns ) {
-            const int intensity = std::ceil( e.get_duration() / e.get_int_dur_factor() );
-            e.set_intensity( std::max( 1, intensity ) );
-        }
-        // Bound new effect intensity by [1, max intensity]
-        if( e.get_intensity() < 1 ) {
-            add_msg_debug( debugmode::DF_CREATURE, "Bad intensity, ID: %s", e.get_id().c_str() );
-            e.set_intensity( 1 );
-        } else if( e.get_intensity() > e.get_max_intensity() ) {
-            e.set_intensity( e.get_max_intensity() );
-        }
         ( *effects )[eff_id][bp] = e;
         if( Character *ch = as_character() ) {
             get_event_bus().send<event_type::character_gains_effect>( ch->getID(), bp.id(), eff_id, intensity );

src/effect.cpp

@@ -1081,16 +1081,10 @@ time_duration effect::get_max_duration() const
 void effect::set_duration( const time_duration &dur, bool alert )
 {
     duration = dur;
-    // Cap to max_duration
-    if( duration > eff_type->max_duration ) {
-        duration = eff_type->max_duration;
-    }
 
-    // Force intensity if it is duration based
-    if( eff_type->int_dur_factor != 0_turns ) {
-        const int intensity = std::ceil( duration / eff_type->int_dur_factor );
-        set_intensity( std::max( 1, intensity ), alert );
-    }
+    clamp_duration();
+
+    apply_int_dur_factor( alert );
 
     add_msg_debug( debugmode::DF_EFFECT, "ID: %s, Duration %s", get_id().c_str(),
                    to_string_writable( duration ) );
@@ -1103,6 +1097,11 @@ void effect::mult_duration( double dur, bool alert )
 {
     set_duration( duration * dur, alert );
 }
+void effect::clamp_duration()
+{
+    duration = std::min( duration, eff_type->max_duration );
+
+}
 
 static int cap_to_size( const int max, int attempt )
 {
@@ -1213,13 +1212,9 @@ int effect::get_effective_intensity() const
 
 int effect::set_intensity( int val, bool alert )
 {
-    if( intensity < 1 ) {
-        // Fix bad intensity
-        add_msg_debug( debugmode::DF_EFFECT, "Bad intensity, ID: %s", get_id().c_str() );
-        intensity = 1;
-    }
+    clamp_intensity();
 
-    val = std::max( std::min( val, eff_type->max_intensity ), 0 );
+    val = std::clamp( val, 0, eff_type->max_intensity );
     if( val == intensity ) {
         // Nothing to change
         return intensity;
@@ -1243,11 +1238,31 @@ int effect::set_intensity( int val, bool alert )
     return intensity;
 }
 
+int effect::clamp_intensity()
+{
+    if( intensity < 1 ) {
+        add_msg_debug( debugmode::DF_CREATURE, "Bad intensity, ID: %s", eff_type->id.c_str() );
+        intensity = 1;
+    } else if( intensity > eff_type->max_intensity ) {
+        intensity = eff_type->max_intensity;
+    }
+    return intensity;
+}
+
 int effect::mod_intensity( int mod, bool alert )
 {
     return set_intensity( intensity + mod, alert );
 }
 
+int effect::apply_int_dur_factor( bool alert )
+{
+    if( eff_type->int_dur_factor != 0_turns ) {
+        const int new_intensity = std::ceil( duration / eff_type->int_dur_factor );
+        set_intensity( std::max( 1, new_intensity ), alert );
+    }
+    return intensity;
+}
+
 const std::vector<trait_id> &effect::get_resist_traits() const
 {
     return eff_type->resist_traits;

src/effect.h

@@ -277,6 +277,9 @@ class effect
             eff_type( peff_type ), duration( dur ), bp( part ),
             permanent( perm ), intensity( nintensity ), start_time( nstart_time ),
             source( source ) {
+            clamp_duration();
+            apply_int_dur_factor();
+            clamp_intensity();
         }
         effect( const effect & ) = default;
         effect &operator=( const effect & ) = default;
@@ -318,6 +321,8 @@ class effect
         void mod_duration( const time_duration &dur, bool alert = false );
         /** Multiplies the duration, capping at max_duration. */
         void mult_duration( double dur, bool alert = false );
+        /** Caps duration at max_duration. */
+        void clamp_duration();
 
         std::vector<vitamin_applied_effect> vit_effects( bool reduced ) const;
 
@@ -353,6 +358,12 @@ class effect
          */
         int set_intensity( int val, bool alert = false );
 
+        /**
+         * Clamps intensity of effect to range [1..max_intensity]
+         * @return new clamped intensity of the effect
+         */
+        int clamp_intensity();
+
         /**
          * Modify intensity of effect capped by range [1..max_intensity]
          * @param mod Amount to increase current intensity by
@@ -361,6 +372,12 @@ class effect
          */
         int mod_intensity( int mod, bool alert = false );
 
+        /**
+         * Set intensity of effect if it is duration based
+         * @return potentially updated intensity of the effect
+         */
+        int apply_int_dur_factor( bool alert = false );
+
         /** Returns the string id of the resist trait to be used in has_trait("id"). */
         const std::vector<trait_id> &get_resist_traits() const;
         /** Returns the string id of the resist effect to be used in has_effect("id"). */

src/vehicle.cpp

@@ -1580,14 +1580,6 @@ void vehicle::add_effect( const effect_source &source, const efftype_id &eff_id,
         effect &e = found_effect->second;
         // If we do, mod the duration, factoring in the mod value
         e.mod_duration( dur * e.get_dur_add_perc() / 100 );
-        // Limit to max duration
-        if( e.get_duration() > e.get_max_duration() ) {
-            e.set_duration( e.get_max_duration() );
-        }
-        // Adding a permanent effect makes it permanent
-        if( e.is_permanent() ) {
-            e.pause_effect();
-        }
     }
 
     if( !found ) {
@@ -1596,23 +1588,7 @@ void vehicle::add_effect( const effect_source &source, const efftype_id &eff_id,
         // Now we can make the new effect for application
         effect e( effect_source( source ), &type, dur, bodypart_str_id::NULL_ID(), permanent, intensity,
                   calendar::turn );
-        // Bound to max duration
-        if( e.get_duration() > e.get_max_duration() ) {
-            e.set_duration( e.get_max_duration() );
-        }
 
-        // Force intensity if it is duration based
-        if( e.get_int_dur_factor() != 0_turns ) {
-            const int intensity = std::ceil( e.get_duration() / e.get_int_dur_factor() );
-            e.set_intensity( std::max( 1, intensity ) );
-        }
-        // Bound new effect intensity by [1, max intensity]
-        if( e.get_intensity() < 1 ) {
-            add_msg_debug( debugmode::DF_CREATURE, "Bad intensity, ID: %s", e.get_id().c_str() );
-            e.set_intensity( 1 );
-        } else if( e.get_intensity() > e.get_max_intensity() ) {
-            e.set_intensity( e.get_max_intensity() );
-        }
         effects[eff_id] = e;
     }
 }

tests/effect_test.cpp

@@ -57,29 +57,40 @@ static const vitamin_id vitamin_test_vitx( "test_vitx" );
 // effect::get_start_time
 //
 // Create an `effect` object with given parameters, and check they were initialized correctly.
-static void check_effect_init( const std::string &eff_name, const time_duration &dur,
-                               const std::string &bp_name, const bool permanent, const int intensity,
+static void check_effect_init( const std::string &eff_name,
+                               const time_duration &requested_dur, const time_duration &expected_dur,
+                               const std::string &bp_name, const bool permanent,
+                               const int requested_int, const int expected_int,
                                const time_point &start_time )
 {
     const efftype_id eff_id( eff_name );
     const effect_type &type = eff_id.obj();
     const bodypart_str_id bp( bp_name );
 
-    effect effect_obj( effect_source::empty(), &type, dur, bp, permanent, intensity, start_time );
+    effect effect_obj( effect_source::empty(), &type, requested_dur, bp, permanent, requested_int,
+                       start_time );
 
-    CHECK( dur == effect_obj.get_duration() );
+    CHECK( expected_dur == effect_obj.get_duration() );
     CHECK( bp.id() == effect_obj.get_bp() );
     CHECK( permanent == effect_obj.is_permanent() );
-    CHECK( intensity == effect_obj.get_intensity() );
+    CHECK( expected_int == effect_obj.get_intensity() );
     CHECK( start_time == effect_obj.get_start_time() );
 }
 
 TEST_CASE( "effect_initialization_test", "[effect][init]" )
 {
     // "debugged" effect is defined in data/mods/TEST_DATA/effects.json
-    check_effect_init( "debugged", 1_days, "head", false, 5, calendar::turn_zero );
-    check_effect_init( "bite", 1_minutes, "torso", true, 2, calendar::turn );
-    check_effect_init( "grabbed", 1_turns, "arm_r", false, 1, calendar::turn + 1_hours );
+    // "debugged" effect has max_duration 1 hour, so 1 day gets clamped
+    check_effect_init( "debugged", 1_days, 1_hours, "head", false, 5, 5, calendar::turn_zero );
+    check_effect_init( "debugged", 1_minutes, 1_minutes, "head", false, 5, 5, calendar::turn_zero );
+    // "bite" effect has int_dur_factor, so intensity is calculated from duration regardless of requested intensity
+    check_effect_init( "bite", 60_minutes, 60_minutes, "torso", true, 99, 2, calendar::turn );
+    check_effect_init( "bite", 30_minutes, 30_minutes, "torso", true, 99, 1, calendar::turn );
+    check_effect_init( "grabbed", 1_turns, 1_turns, "arm_r", false, 100, 100,
+                       calendar::turn + 1_hours );
+    check_effect_init( "grabbed", 100_turns, 100_turns, "arm_r", false, 1, 1,
+                       calendar::turn + 1_hours );
+    check_effect_init( "grabbed", 1_turns, 1_turns, "arm_r", false, 10, 10, calendar::turn + 1_hours );
 }
 
 // Effect duration