Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase granularity of activity tracking to per turn. #45316

Merged

Conversation

kevingranade
Copy link
Member

@kevingranade kevingranade commented Nov 9, 2020

Summary

SUMMARY: None

Purpose of change

Increase activity tracking granularity to per turn.
Simplify character class slightly by extracting activity and weariness logic to a helper class.

Describe the solution

Extract the weariness and activity tracking code into a separate class.
Also activity level was not being saved or loaded, do that.
Increase granularity of tracking to per-turn.
Maintains a rolling average of activity level for bmr calculation, and also an instantaneous activity level for other uses.

Describe alternatives you've considered

I'm not at all happy with the names, activity_tracker is okish, but activity_history is just bad.

Testing

Add a number of unit tests, insure the existing weariness and calorie expenditure tests continue to work.
Run around and do active stuff manually and validate the results.

@Aivean Aivean added [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style labels Nov 9, 2020
@kevingranade kevingranade force-pushed the extract-weary-and-activity branch from fe794f0 to 5dace2d Compare November 13, 2020 07:18
@kevingranade
Copy link
Member Author

kevingranade commented Nov 13, 2020

Updated with WIP adjustments to make activity ticks more granular by tracking them on a turn-by-turn basis and then applying them once every 5 minutes.

I'm kind of stuck what this does that breaks weariness so badly, it absolutely DESTROYS the weariness tests because weariness passes an inflection point and then just keeps climbing rapidly, I'm not sure what I did to make this happen.
Posted this and then immediately realized what it was and fixed it.

This works, but needs some polish and picking the commits apart, maybe a little more testing

src/activity_tracker.h Outdated Show resolved Hide resolved
@kevingranade kevingranade force-pushed the extract-weary-and-activity branch 2 times, most recently from 5af5fed to bb1b999 Compare November 15, 2020 02:18
@kevingranade kevingranade changed the title Extract activity and weariness tracking to a helper class. Increase granularity of activity tracking to per turn. Nov 15, 2020
@kevingranade kevingranade marked this pull request as ready for review November 15, 2020 02:24
src/activity_tracker.cpp Outdated Show resolved Hide resolved
src/activity_tracker.cpp Outdated Show resolved Hide resolved
src/activity_tracker.cpp Outdated Show resolved Hide resolved
src/character.h Show resolved Hide resolved
@kevingranade kevingranade force-pushed the extract-weary-and-activity branch from bb1b999 to d2bce00 Compare November 20, 2020 03:25
@ZhilkinSerg ZhilkinSerg self-assigned this Nov 23, 2020
@kevingranade kevingranade force-pushed the extract-weary-and-activity branch 2 times, most recently from 7e3e6c9 to 53fb7a5 Compare January 17, 2021 23:10
Comment on lines 107 to 132
accumulated_activity += current_activity;
// skipped turns?
current_activity = new_level;
num_events++;
Copy link
Member

@anothersimulacrum anothersimulacrum Jan 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should look like this to account for the turns thing

Suggested change
accumulated_activity += current_activity;
// skipped turns?
current_activity = new_level;
num_events++;
int turns_moved = calendar::turn - current_turn;
accumulated_activity += current_activity * turns_moved;
current_activity = new_level;
num_events += turns_moved;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort of, we can do number of turns, but the activity level if anything should be NO_EXERCISE, representative of the fact that no activity was logged in the interim.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd have to look through to see when skipped turns trigger, but this is the correct behavior for what I was considering, when an activity takes more than a characters moves to complete. E.g. if I drag furniture and it takes me 6 turns (but it is not an activity), I should be counted as whatever activity level for 6 turns, instead of just 1, and nothing for 5 turns.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The activity should be logging its exertion level every turn.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am talking about non-activity actions like that. Dragging is now an activity, but if it were not this would be the correct behavior.
I am not sure how many of these we have, however. Melee combat comes to mind as somewhere this could pop up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, anything the player does should log an activity, and anything that takes more than one turn should log each turn.
If that isn't happening, we can fix it.

@kevingranade kevingranade force-pushed the extract-weary-and-activity branch from 53fb7a5 to 0d1c7e7 Compare January 18, 2021 04:00
@kevingranade kevingranade force-pushed the extract-weary-and-activity branch 2 times, most recently from 5832df9 to 41c1498 Compare February 3, 2021 06:18
@kevingranade
Copy link
Member Author

Rebased and fixed the failing starve test. It was being thrown off by the rolling average of activity level, so if another test happened right before it, it was picking up an unusually high activity level.

@kevingranade kevingranade force-pushed the extract-weary-and-activity branch from 31f83f6 to 8af1803 Compare March 21, 2021 00:19
@kevingranade
Copy link
Member Author

Rebased on latest master and should be ready to go, I incorporated @anothersimulacrum 's activity level reporting feature into the activity tracker.

For future consideration, I want to unify move point, stamina, and weariness tracking behind a method called Character::expend_effort( int moves, float exertion_level ) That way we never expend moves without logging effort, and we never log effort without expending moves, and the product of moves spent and exertion level should be sufficient to determine stamina and fatigue for the action as well as being the sole place to spend moves.

@actual-nh
Copy link
Contributor

For future consideration, I want to unify move point, stamina, and weariness tracking behind a method called Character::expend_effort( int moves, float exertion_level ) That way we never expend moves without logging effort, and we never log effort without expending moves, and the product of moves spent and exertion level should be sufficient to determine stamina and fatigue for the action as well as being the sole place to spend moves.

Spending of calories?

@kevingranade
Copy link
Member Author

Spending of calories?

Yes, exertion level would feed into calorie expenditure.

@actual-nh
Copy link
Contributor

actual-nh commented Mar 21, 2021

This PR has some of the same thinking as #47653, namely trying to smooth out some of the current fluctuations due to things happening every X minutes. If it goes in prior to #47653, it will unfortunately require a bit of a rewrite on my part of #47653 - or vice-versa, if #47653 goes in first - but that would just be an annoyance.

For the future, the finer activity tracking would make it possible to have less-than-full intervals for figuring out weariness recovery - one could benefit from 3 minutes of rest, for instance.

@actual-nh
Copy link
Contributor

BTW, the two failing builds are both at the activity_counter_from_1_to_300 test; tracker.average_activity() vs expected_activity 1.00863f vs 150.5f. (The Travis one is for mingw-w64.)

@anothersimulacrum anothersimulacrum self-assigned this Mar 21, 2021
@anothersimulacrum
Copy link
Member

I've assigned myself to this so I (hopefully) remember to review it.

@kevingranade
Copy link
Member Author

I THINK what's happening with that failing test is that calendar::turn is relatively advanced, and the loop in activity_tracker::new_turn() is incorporating the difference between calendar::turn_zero and calendar::turn into the tracker, resulting in the activity score getting greatly depressed. The thing I don't get is why it isn't always happening.

Comment on lines 105 to 114
while( turns <= task.interval && !task.instantaneous() ) {
// Start each turn with a fresh set of moves
guy.moves = 100;
task.do_turn( guy );
// Advance a turn
calendar::turn += 1_turns;
turns += 1_seconds;
// Consume food, become weary, etc
guy.update_body();
// Start each turn with a fresh set of moves
guy.moves = 100;
task.do_turn( guy );
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see why putting the do_turn after advancing turns makes sense. But why update body prior to do_turn?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because that's the order in which it occurs in game::do_turn().
Technically moves are incremented at the end of the turn instead of the start, but update_body() shouldn't be interacting with moves, so it shouldn't matter.

@kevingranade kevingranade force-pushed the extract-weary-and-activity branch 2 times, most recently from ac8bae1 to 4291394 Compare March 24, 2021 00:55
@kevingranade kevingranade force-pushed the extract-weary-and-activity branch from 4291394 to 035b0a9 Compare March 25, 2021 16:53
src/activity_tracker.h Outdated Show resolved Hide resolved
Comment on lines 5654 to 5638
int amount = weariness();
int amount = activity_history.weariness();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like with the change to Character::weariness() this could be left alone.

src/activity_tracker.h Outdated Show resolved Hide resolved
src/activity_tracker.h Outdated Show resolved Hide resolved
src/activity_tracker.h Outdated Show resolved Hide resolved
src/character.cpp Outdated Show resolved Hide resolved
@ZhilkinSerg ZhilkinSerg merged commit 8bc8877 into CleverRaven:master Mar 31, 2021
@kevingranade kevingranade deleted the extract-weary-and-activity branch April 2, 2021 22:52
actual-nh added a commit to actual-nh/Cataclysm-DDA that referenced this pull request Jun 4, 2021
avatar::log_activity_level is now getting fed (floating-point) numbers
that don't match the exact activity levels, due to CleverRaven#45316 making the
activity level per-turn then averaging activity over each 5-minute
period. The activity diary test (in tests/char_biometrics_test.cpp)
was not spotting this since it was still using times in multiples
of 5 minutes. This commit moves around 1 minute of NO_EXERCISE,
keeping the same total number of minutes of each activity level.
Currently, this results in a test failure in which 1 5-minute interval
is missing from each of the activity levels in the diary, since those
5-minute intervals were at intermediate (averaged) levels of activity.
actual-nh added a commit to actual-nh/Cataclysm-DDA that referenced this pull request Jun 5, 2021
This "buckets" activity levels that aren't exactly the predefined
set of levels (due to averaging over 5 minutes) into the closest
predefined level in avatar::total_daily_calories_string. Note that
saving of levels will not be bucketed. This, while useful for close
inspection of intermediate levels via looking at the save file, will
make the file slightly larger, slower to input/output, and (even)
harder to read manually. However, this has been true since CleverRaven#45316 was
merged and nobody seems to have noticed it.
ZhilkinSerg pushed a commit that referenced this pull request Jun 8, 2021
* Activity diary test now detects float-keying problem

avatar::log_activity_level is now getting fed (floating-point) numbers
that don't match the exact activity levels, due to #45316 making the
activity level per-turn then averaging activity over each 5-minute
period. The activity diary test (in tests/char_biometrics_test.cpp)
was not spotting this since it was still using times in multiples
of 5 minutes. This commit moves around 1 minute of NO_EXERCISE,
keeping the same total number of minutes of each activity level.
Currently, this results in a test failure in which 1 5-minute interval
is missing from each of the activity levels in the diary, since those
5-minute intervals were at intermediate (averaged) levels of activity.

* Process intermediate activity levels for debug output

This "buckets" activity levels that aren't exactly the predefined
set of levels (due to averaging over 5 minutes) into the closest
predefined level in avatar::total_daily_calories_string. Note that
saving of levels will not be bucketed. This, while useful for close
inspection of intermediate levels via looking at the save file, will
make the file slightly larger, slower to input/output, and (even)
harder to read manually. However, this has been true since #45316 was
merged and nobody seems to have noticed it.

* Correct logic error in attempted fix.

I forgot to add the number of 5-minute intervals and was instead adding
just one. It still doesn't quite work right (and I'll need to adjust
the expected gained/spent/total caloric values), but it's a lot closer.

* Adjust expected numbers for mixed intervals

The new results in terms of minutes are actually now correct; the
difference is because the last 5 minutes are 4 minutes of
EXTRA_EXERCISE and 1 minute of NO_EXERCISE, which averages out to
ACTIVE_EXERCISE (410 + 11 = 41; /5 = 8.2, and ACTIVE_EXERCISE = 8.0).
The rest is likely roundoff error, although it still bothers me a bit.

Co-authored-by: actual-nh <[email protected]>
@ZhilkinSerg ZhilkinSerg removed their assignment Jul 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants