Allow for specialization of activity parameters #37450
Conversation
ifreund
added
[C++]
|
Just realized that I've forgotten to handle save migration for in progress legacy activities, So I'm marking this as WIP for now. |
ifreund
changed the title
|
Migration is now handled through a new ACT_MIGRATION_CANCEL activity which This may result in inconvenience when a save is migrated, but should avoid This also avoids the tedious and error-prone task of writing custom |
ifreund
changed the title
|
While testing, my game freezes when I try to move all in advanced inventory using the default key , I cannot reproduce it when compiling from master, only from this branch. Can you reproduce? |
|
@snipercup thanks, I forgot to migrate the move all function. Should be fixed now. |
|
Can confirm, the bug I reported is no longer there. |
|
I continued playing for a while but didnt find any bugs related to this pr. |
| if( filtered_any_bucket ) { | ||
| add_msg( m_info, _( "Skipping filled buckets to avoid spilling their contents." ) ); | ||
| } | ||
|
|
There was a problem hiding this comment.
This currently adds a lot of code duplication in the advanced inventory, but that will be remedied when I migrate ACT_PICKUP and ACT_WEAR to use the actor system.
| class player_activity | ||
| { | ||
| private: | ||
| activity_id type; | ||
| cata::clone_ptr<activity_actor> actor; |
There was a problem hiding this comment.
Does player_activity need to be copyable? Could this just be a unique_ptr?
There was a problem hiding this comment.
It may not be strictly necessary, but it seemed easier than tracking down all the places we are currently doing copy construction and copy assignment to add an std::move() thanks to the cata::clone_ptr type you added. I guess I didn't think about it too deeply but don't see a big drawback in keeping it copyable.
There was a problem hiding this comment.
Yeah, if it's already being copied sometimes then that's probably fine.
This commit introduces the activity_actor abstract class and the infrastructure needed to integrate with the current player_activity system. This commit also migrates ACT_MOVE_ITEMS to use this new actor system as and example and a test of the new system. The activity actor system allows for each actor to declare its own state variables which is an improvement over the status quo of each activity making do with the preselected state variables available in player_activity. The migration of activities to this new system can be done in an incremental way until all activities have been migrate to the actor system, at which point the legacy infrastructure can be removed.
Migration is handled through a new ACT_MIGRATION_CANCEL activity which clears the backlog, resets the state of the avatar or npc, and sets itself to null the first time its do_turn is called. This may result in inconvience when a save is migrated, but should avoid any lasting negative effects on the save. This also avoids the tedious and error-prone task of writing custom migration code for each activity migrated to the activity_actor system.
|
rebased to resolve conflicts |
Summary
SUMMARY: Infrastructure "Allow for specialization of activity parameters"
Purpose of change
Currently player activities can only store state during execution by using one of
the already available members of
player_activityor creating a new one if thedesired type is not already available.
Describe the solution
This commit introduces the activity_actor abstract class and the
infrastructure needed to integrate with the current player_activity system.
This commit also migrates ACT_MOVE_ITEMS to use this new actor system
as and example and a test of the new system.
The activity actor system allows for each actor to declare its own
state variables allowing specialization for the type of activity being
preformed and much more readable code
The migration of activities to this new system can be done in an
incremental way until all activities have been migrated to the actor
system at which point the legacy infrastructure can be removed.
Describe alternatives you've considered
Alternatives to this approach include creating more generic parameter types
in
player_actvity, perhaps usingcata_varaint. However, such an approachis still limited in the specialization available compared to this system.
Testing
Hauling and moving items with the advanced inventory are still fully functional.
Autosaves during hauling can be reloaded without errors and result in resumption
of the activity.
Additional context
I'm not entirely happy with how the deserialization of activity actors is handled
in the code. If anyone has suggestions on that front or on the system in general
I'd love to hear them.