(c++) Editing activity_actor.h causes too much recompilation

[sacrificerXY]

So activity_actor.h is included in player_activity.h, which is then included in Character.h. Now I'm no expert but I'm pretty sure this is suboptimal. Afaik, this is the one and only dependency chain which has a large effect.

One possible solution I tried was forwarding activity_actor in player_activity.h to break the dependency. But it doesn't work because it is used in a clone_ptr which requires a full definition. This is the only thing preventing the forward declaration. Idk if things could be restructured to achieve this so I hope someone else can look at it.

Another minor solution which could help is to modularize activity_actor.h. It is being included in some cpp files because of the need to access 1 or 2 activity_actors. Creating/migrating new activities then affects these files even though they logically shouldn't be. Splitting them up should minimize this. Maybe put them in their own subfolder?

[kevingranade]

There are a ton of these, and we're slowly trying to chip away at them, but as you've found, a lot of them require non trivial refactoring.

It looks like we might be able to move the activity_actor subclasses into a new header file and include these just where needed?

I'm doing some test builds now.

[anothersimulacrum]

In addition to @kevingranade 's approach (which is probably better), it's also possible to pimpl the player_activities.
It would look something like this:

diff --git a/src/character.cpp b/src/character.cpp
index dd2b93390e..ecaaeedf50 100644
--- a/src/character.cpp
+++ b/src/character.cpp
@@ -1013,6 +1013,11 @@ player_activity Character::get_stashed_activity() const
     return stashed_outbounds_activity;
 }

+void Character::set_stashed_activity( const player_activity &act )
+{
+    set_stashed_activity( act, player_activity() );
+}
+
 void Character::set_stashed_activity( const player_activity &act, const player_activity &act_back )
 {
     stashed_outbounds_activity = act;
@@ -12372,6 +12377,11 @@ nc_color Character::bodytemp_color( const bodypart_id &bp ) const
     return color;
 }

+void Character::set_destination( const std::vector < tripoint &route )
+{
+    set_destination( route, player_activity() );
+}
+
 void Character::set_destination( const std::vector<tripoint> &route,
                                  const player_activity &new_destination_activity )
 {
diff --git a/src/character.h b/src/character.h
index c6ec8d499b..c098685b88 100644
--- a/src/character.h
+++ b/src/character.h
@@ -37,7 +37,6 @@
 #include "memory_fast.h"
 #include "optional.h"
 #include "pimpl.h"
-#include "player_activity.h"
 #include "pldata.h"
 #include "point.h"
 #include "recipe.h"
@@ -71,6 +70,7 @@ class monster;
 class nc_color;
 class npc;
 class player;
+class player_activity;
 class player_morale;
 class proficiency_set;
 class recipe_subset;
@@ -1942,9 +1942,9 @@ class Character : public Creature, public visitable<Character>
         bool in_vehicle = false;
         bool hauling = false;

-        player_activity stashed_outbounds_activity;
-        player_activity stashed_outbounds_backlog;
-        player_activity activity;
+        pimpl<player_activity> stashed_outbounds_activity;
+        pimpl<player_activity> stashed_outbounds_backlog;
+        pimpl<player_activity> activity;
         std::list<player_activity> backlog;
         cata::optional<tripoint> destination_point;
         pimpl<inventory> inv;
@@ -2039,8 +2039,9 @@ class Character : public Creature, public visitable<Character>
         void cancel_activity();
         void cancel_stashed_activity();
         player_activity get_stashed_activity() const;
+        void set_stashed_activity( const player_activity &act );
         void set_stashed_activity( const player_activity &act,
         void set_stashed_activity( const player_activity &act,
-                                   const player_activity &act_back = player_activity() );
+                                   const player_activity &act_back );
         bool has_stashed_activity() const;
         bool can_stash( const item &it );
         bool can_stash_partial( const item &it );
@@ -2624,8 +2625,9 @@ class Character : public Creature, public visitable<Character>
         bool has_weapon() const override;
         void shift_destination( const point &shift );
         // Auto move methods
+        void set_destination( const std::vector<tripoint> &route );
         void set_destination( const std::vector<tripoint> &route,
-                              const player_activity &new_destination_activity = player_activity() );
+                              const player_activity &new_destination_activity );
         void clear_destination();
         bool has_distant_destination() const;

@@ -2827,7 +2829,7 @@ class Character : public Creature, public visitable<Character>
         // a cache of all active enchantment values.
         // is recalculated every turn in Character::recalculate_enchantment_cache
         pimpl<enchantment> enchantment_cache;
-        player_activity destination_activity;
+        pimpl<player_activity> destination_activity;
         /// A unique ID number, assigned by the game class. Values should never be reused.
         character_id id;

[kevingranade]

PR up to address this at #45967

[sacrificerXY]

Damn. I genuinely thought splitting the header is a minor solution. But it seems it's actually more than sufficient. Welp learned something new today.

[kevingranade]

It depends on the details, in this case the base class is all that is needed almost always and it's tiny, and it pulls in very few headers. With a different set of facts around this splitting out the subclasses would do little to nothing.

A more typical approach would be to split it up into one subclass per file, and that would likely be a win, but at the moment that looks like it's pretty far into diminishing returns. We're a lot more worried about the header files that have hundreds of inclusions instead of the ones with a few dozen.