Fix proficiency books' effect on crafting #48650

eltank · 2021-04-25T17:30:33Z

Summary

Bugfixes "Fix proficiency books' effect on crafting"

Purpose of change

Fixes #48465
PR #46347 added the ability for books to partially mitigate the crafting penalties incurred due to missing proficiencies, as described in FR #44680. However, it introduced some issues.

It changed the crafting UI but didn't actually change the crafting time. If you read recipe::time_to_craft_moves() it's pretty clear that book proficiencies are not used. The total time taken to craft an item when you're missing proficiencies is unaffected by books (verified via gameplay as well). Reported in Mitigated proficiency penalties do not apply correctly to crafting time #48465
It implemented the mitigation formula incorrectly, which would result in books letting you craft faster when missing a proficiency than when you had the proficiency (if they actually took effect). This caused player confusion, e.g. 0.1x as long as normal crafting from mitigated proficiency #46662
When a book provides a proficiency time_factor of 0.1, a malus of 1.5 due to lacking that proficiency should become 1.05 (1 + 0.5 * 0.1) not 0.15 (1.5 * 0.1). The book is supposed to reduce the penalty, not turn it into a bonus.
The formula for stacking mitigation effects from multiple books is also wrong and can result in negative proficiency malus.
With 2 books that provide time_factors of 0.1 and 0.5, the formula from Assisting with proficiencies through books #44680 (as implemented in the current code) yields a combined time_factor of 1 - sqrt((1 - 0.1)^2 + (1 - 0.5)^2) = -0.03, which is nonsense.
The book proficiency mitigation effect on the failure rate was also not getting applied ~~(in addition to the penalty acting as a bonus, see Fix the crafting success formula when crafter is missing proficiencies. #48673)~~

Describe the solution

Modified recipe::time_to_craft_moves() to call proficiency_time_maluses().
See Fix book proficiency mitigation formulas. #48658
See Fix book proficiency mitigation formulas. #48658
Divide the crafting success roll by the recipe's proficiency_time_maluses().

~~Unresolved issues:~~

~~it makes the game slow to a crawl when crafting in the middle of a pile of books and ingredients~~ [fixed]
~~it removes const qualifiers from a bunch of APIs, which feels very wrong~~ [fixed]

The slowdown was due to calling Character::crafting_inventory() from the proficiency_time_maluses() added to recipe::time_to_craft_moves(), which is called many times while crafting from craft_activity_actor::do_turn(). Character::crafting_inventory() is an expensive member function of the player object that needs to scan all nearby items. It's non-const (hence the removal of const qualifiers) because it does caching. But the cache is only valid for one turn, which doesn't help in this situation.
The fix for this was to cache the results of the two rec.batch_time() calls in craft_activity_actor::do_turn(). The cache is invalidated when crafting speed or assistants change, and at 5% progress increments when proficiencies may be gained.

Describe alternatives you've considered

Rolling back #46347. But I like the idea and I really want this to work.

~~Not sure if I need to also check for a change in craft.charges as a condition that invalidates the cache.~~ [no, it doesn't change] On the other hand, it may be possible to remove the check for changing speed and assistants (which may also slow down crafting due to looking around the map) and rely entirely on the 5% progress updates. We might be using the wrong crafting times sometimes, but not for longer than 5% of the crafting progress. But, 5% could be a long time for some recipes.

Comparing floating point numbers in the cache invalidation check is probably not ideal. Maybe use epsilon equality.

Considered changing craft_activity_actor::start() to calculate the total number of moves required there instead of in craft_activity_actor::do_turn(). But the number of moves may change during crafting.

To avoid propagating non-const references, I have 2 ideas for changing Character::crafting_inventory() to be a const member function. One is to put the crafting inventory cache into a static global variable, the other is to use a const_cast inside that function to force-allow modification of the cache object. I can't say that I'm a big fan of either approach.

Testing

Build 2 game binaries, one without this change ("before") and one with ("after").
Using "before", start a new game in evac shelter. Debug all skills to 10 and INT to 20. Debug unlock all recipes. Debug the Life Support, HVAC mutations. Move next to a computer terminal for the light (but not next to any counter, which boosts crafting speed) or spawn and activate an atomic light. Debug kill all monsters. Debug spawn a plank and 2 long strings. Save.

Bring up the crafting menu and look at "short bow".

Due to lacking proficiencies it will take 20 hours (5x increase) as indicated by "Time to complete". Craft the bow, using a stopwatch to measure how long it takes in real time. Observe that it finishes at 4am (20 game hours later). On my computer it takes 30s of real time.

Reload (still using "before"). Spawn "The Bowyer's Buddy", and bring up the crafting UI again.

Although the text indicates that your crafting penalty has changed to 3.8x the "Time to complete" still shows 20 hours. This is the buggy behavior we're trying to fix. Craft the bow. Observe that it finishes at 4am (20 hours later).

Switch to the "after" binary and reload. Verify that the crafting UI looks the same as in "before" without a book, and you can craft the bow in 20 hours (finish at 4am).
To measure performance regression, time the operation. On my computer it took 31 seconds, which agrees with the "before" timing within measurement error.

Reload. Spawn the book. The UI should now look like this:

The "Time to complete" is now 17 hours. (4h base * 4.25 mitigated crafting malus). Craft the bow. Observe that it finishes at around 1 am (17 game hours). On my computer it takes 27s of real time.

Here's a more complicated example with a multiple-proficiency craft and a pile of books.

Before

After

Crafting time reduced from 3 days to 1 day 18 hours by the presence of proficiency-mitigating books.

Additional context

For my next PR I plan to implement another of Erk's ideas from #44680:
Once we have the ability to reduce proficiency penalties by a factor, the same infrastructure could and should be used to make proficiency penalties gradually fall off as you get close to the necessary xp to have the proficiency.

eltank · 2021-04-26T04:52:13Z

I am moving fixes 2 and 3 to a separate PR (#48658), since fixing 1 is going to take a while. Please review that first.

Salty-Panda · 2021-04-26T07:54:04Z

Looking good.
About cached_inv, maybe chek book_proficiency_bonuses at the start/resume of crafting and put it in proficiency?
Not sure how simple will it be to pass it from there to the crafting function though.

eltank · 2021-04-26T09:23:20Z

I found a simple solution that seems to work. I'm caching the calls to rec.batch_time() in craft_activity_actor::do_turn(). The cache is invalidated when crafting speed or assistants change, and at 5% progress increments when proficiencies may be gained. Playtesting confirms that the massive slowdown is eliminated by doing this. I used debug messages to verify that the cache is re-populated only at the 5% increment marks under normal conditions. PR updated.

ferociousdork · 2021-04-26T21:42:30Z

Still looking for advice regarding the non-const references (see also alternatives considered).

Standard practice would be to make the members involved in the caching mutable so you can keep the methods const.

eltank · 2021-04-27T00:22:07Z

Thanks, I'm ashamed to admit that I didn't know C++ had a "mutable" keyword. Sounds good, I'll give that a shot.

eltank · 2021-04-27T06:10:17Z

I was stuck in "const hell" for a while because item_location code is really not designed to work with const item pointers. I bailed out because I think it would take a lot of work to change that.
In the end I ended up marking the Character inventory cache fields "mutable" but I still needed to use a const_cast in order to call Character::all_items_loc() from Character::crafting_inventory(). It was either that or mark "worn" and "weapon" as mutable too, which seemed worse.

I also noticed that the crafting failure formula is making recipes X times more likely to succeed instead of X times less likely when you're missing proficiencies (and it's also not affected by books), so I need to investigate/fix that before marking the PR as ready.
I will fix the formula in a separate PR (#48673) though since it could make a lot of players angry -- so far nobody complained about the harsh proficiency penalties on crafting success rates because they were not taking effect, but once it's fixed crafting without required proficiencies will become a lot harder. But I'll handle the book-based failure mitigation in this PR.

eltank · 2021-04-27T07:31:20Z

All the fixes are in, ready for review now. I would prefer to get #48658 and #48673 in first (although this PR contains the work from the others too). That would allow me to rebase and shrink this PR (actually not sure if rebase is needed or GitHub will recompute the diff against master's head automatically).

ghost · 2021-05-27T08:00:09Z

There is no bonus to mitigation, if you mitigate it can only to at best to 0x, which pushes it closes to the normal rates of 50% chance success or the value of 1. Any increment of the multiplier will increment the value of 1 a degree higher. It still takes more time and is less effective than having proficiency.

It's just unclear to people because they don't know where the normal point exists between the calculations. It's tied to where you have a 50% success chance. Having proficiency already provides a bonus effect. You can even see the difference in time between the normal value and the proficiency mitigating closer to 0x.

It's where all the multipliers are being scaled off of, which is why you get some weird numbers sometimes, but it should all just be functioning normally, they just don't increment normally in the way you think they should, especially when mitigated.

The bonus tied to having a proficiency is considered a -.5x time multiplier with a 1x more likely to succeed than normal. You treat 1x just like you would treat 100%.

This should be 100% (after being fixed) but it's currently 75% now.

It's confusing to show for players for reason, the time multiplier isn't really time, it just from the normal recipe the scale the rest of the craft off. You can treat it as a bonus though if it makes it more clear. It does decrease the duration to craft though but is more related to how much progress to the degree of item completion you're awarded per tick.

I-am-Erk · 2021-05-28T16:38:34Z

Before I add this to the pile, are there any string changes for translation in here? I see on the other pr you were conscious of the concerns there.

actual-nh · 2021-05-28T16:40:55Z

Before I add this to the pile, are there any string changes for translation in here? I see on the other pr you were conscious of the concerns there.

I'm not seeing any.

actual-nh · 2021-05-28T16:43:43Z

@eltank: Rebasing looks likely to fix the clang build analyzer and clang-tidy fixes; see #48977. Checking Travis; unrelated error. EDIT: OTOH, rebasing is going to cause other test errors due to #49048 being needed, so it may be best to wait until that's in.

eltank · 2021-05-28T20:31:12Z

Correct, I avoided modifying any strings in order to make it possible to put this in 0.F.
@actual-nh Sorry, I didn't see your last edit so I rebased my PRs on the current version of master. I'll rebase again after that fix is in.

eJ0opb · 2021-05-29T00:09:40Z

Correct, I avoided modifying any strings in order to make it possible to put this in 0.F.
@actual-nh Sorry, I didn't see your last edit so I rebased my PRs on the current version of master. I'll rebase again after that fix is in.

The commit that made that #49048 necessary was reverted on master.

eltank · 2021-05-29T00:55:19Z

The commit that made that #49048 necessary was reverted on master.

Thanks for the clarification. If the reversion you're talking about is #49053 then I already picked it up when I rebased so I expect to not need another rebase. I see that the tests appear to be passing (though not all have finished).

Also makes Character::crafting_inventory() const.

eltank · 2021-05-30T06:16:29Z

I've updated the PR (rebased on master now that #48658 has been merged), reducing the number of diffs.

jbytheway

I think the const additions here are generally a step in the right direction, so thanks for that.

I haven't understood the details of exactly how your other changes affect the logic. I'd be happier if you could add a unit test that fails with the old code but passes with the new code. There's already some crafting tests that you could hopefully use for inspiration.

src/character.h

jbytheway · 2021-05-30T11:36:14Z

src/crafting.cpp

@@ -561,7 +554,7 @@ const inventory &Character::crafting_inventory( const tripoint &src_pos, int rad
        cached_crafting_inventory->form_from_map( inv_pos, radius, this, false, clear_path );
    }

-    for( const item_location &it : all_items_loc() ) {
+    for( const item_location &it : const_cast<Character *>( this )->all_items_loc() ) {


So the morally correct thing here would be to introduce a new const_item_location class which is like item_location, except with const access to the underlying item, and then have a const overload of all_items_loc which uses that. It looks like cached_crafting_inventory.add_item copies its argument, so I think that should work.

That's the sort of thing that could reasonably go in a follow-up PR.

I still have a bit of PTSD from the last time I tried propagating const-ness to item_location. I would definitely prefer to leave it for another PR since even if I succeed with this new approach it will introduce a lot of extra diffs.

Can you add a comment suggesting that this would be a good thing to add, perhaps?

eltank · 2021-05-30T20:23:04Z

I'll work on adding a unit test in crafting_test.cpp today. In order to avoid brittle expectations such as finishing the craft in a specific number of turns I'll probably just compare the time taken to craft with and without a book present. [Done]

actual-nh · 2021-05-31T01:08:16Z

I'll work on adding a unit test in crafting_test.cpp today. In order to avoid brittle expectations such as finishing the craft in a specific number of turns I'll probably just compare the time taken to craft with and without a book present. [Done]

Is a further check vs how long with the actual proficiency (that that's less time still) desirable?

eltank · 2021-05-31T02:31:52Z

I'll work on adding a unit test in crafting_test.cpp today. In order to avoid brittle expectations such as finishing the craft in a specific number of turns I'll probably just compare the time taken to craft with and without a book present. [Done]

Is a further check vs how long with the actual proficiency (that that's less time still) desirable?

Good idea. The time taken with the book should be strictly between the time taken with no prof and no mitigation and time taken with the prof. [Done]

eltank · 2021-05-31T06:22:58Z

I haven't understood the details of exactly how your other changes affect the logic. I'd be happier if you could add a unit test that fails with the old code but passes with the new code. There's already some crafting tests that you could hopefully use for inspiration.

Summary of key changes:

recipe.cpp:70 (calls proficiency_time_maluses(), which incorporates book mitigation, instead of applying unmitigated penalties)
return time * proficiency_time_maluses( guy );
crafting.cpp:1015 (calls proficiency_failure_maluses() instead of applying unmitigated penalties)
return ( skill_roll / diff_roll ) / making.proficiency_failure_maluses( *this );

The caching in craft_activity_actor::do_turn (activity_actor.cpp:1769) was necessary to fix a performance issue resulting from the above changes.
Everything else is code cleanup because I didn't want to drop more const qualifiers from existing methods and I added back some of the const qualifiers removed by #46347.

The newly added test fails on master because of the first check (mitigated_time_taken < unmitigated_time_taken) since book proficiency mitigations have no effect in that branch so the times are equal.
I don't have a test for checking that the failure rate is mitigated. I thought about simulating the crafting operation but that seems brittle due to the non-determinism of the failure calculations.

I-am-Erk

As far as I can tell this is good to go. I would not mind seeing a few more comments on it for future editors. I will clear with a more versed person before merging.

I-am-Erk · 2021-06-03T15:11:01Z

src/crafting.cpp

@@ -561,7 +554,7 @@ const inventory &Character::crafting_inventory( const tripoint &src_pos, int rad
        cached_crafting_inventory->form_from_map( inv_pos, radius, this, false, clear_path );
    }

-    for( const item_location &it : all_items_loc() ) {
+    for( const item_location &it : const_cast<Character *>( this )->all_items_loc() ) {


Can you add a comment suggesting that this would be a good thing to add, perhaps?

* Fix proficiency books' effect on crafting * Cache recipe batch time in craft_activity_actor::do_turn() * Make crafting time functions const and/or operate on const Character. Also makes Character::crafting_inventory() const. * add a unit test * also check that proficiency mitigation is not as good as having prof * use a struct to encapsulate the crafting inventory cache * clean up one hack * Add TODO for getting rid of const_cast

actual-nh requested a review from anothersimulacrum April 25, 2021 17:48

actual-nh added <Bugfix> This is a fix for a bug (or closes open issue) Crafting / Construction / Recipes Includes: Uncrafting / Disassembling Items / Item Actions / Item Qualities Items and how they work and interact Mechanics: Effects / Skills / Stats Effects / Skills / Stats labels Apr 25, 2021

eltank mentioned this pull request Apr 26, 2021

Fix book proficiency mitigation formulas. #48658

Merged

eltank mentioned this pull request Apr 26, 2021

Mitigated proficiency penalties do not apply correctly to crafting time #48465

Closed

eltank mentioned this pull request Apr 27, 2021

Fix the crafting success formula when crafter is missing proficiencies. #48673

Merged

eltank marked this pull request as ready for review April 27, 2021 07:25

eltank changed the title ~~[CR] Fix proficiency books' effect on crafting~~ Fix proficiency books' effect on crafting Apr 28, 2021

eltank force-pushed the fix_book_prof_mitigation branch 2 times, most recently from f695f18 to 943da8d Compare May 2, 2021 16:59

eltank force-pushed the fix_book_prof_mitigation branch from 943da8d to ffdbbcd Compare May 20, 2021 21:50

actual-nh requested a review from kevingranade May 20, 2021 22:07

Kelenius mentioned this pull request May 21, 2021

Proficiencies, crafting, failure chance, UI, etc #48981

Closed

actual-nh requested review from I-am-Erk and removed request for kevingranade May 28, 2021 16:32

eltank force-pushed the fix_book_prof_mitigation branch from ffdbbcd to 36c62f9 Compare May 28, 2021 20:26

eltank force-pushed the fix_book_prof_mitigation branch from 0f8ef84 to d364d4d Compare May 28, 2021 21:53

eltank added 3 commits May 29, 2021 22:54

Fix proficiency books' effect on crafting

5fd8296

Cache recipe batch time in craft_activity_actor::do_turn()

a2070be

Make crafting time functions const and/or operate on const Character.

ec35ae9

Also makes Character::crafting_inventory() const.

eltank force-pushed the fix_book_prof_mitigation branch from f6cc2c1 to ec35ae9 Compare May 30, 2021 06:12

jbytheway reviewed May 30, 2021

View reviewed changes

add a unit test

89c47d0

eltank force-pushed the fix_book_prof_mitigation branch 2 times, most recently from 99f78f1 to 959440e Compare May 31, 2021 02:55

also check that proficiency mitigation is not as good as having prof

f46c907

eltank force-pushed the fix_book_prof_mitigation branch from 959440e to f46c907 Compare May 31, 2021 03:05

use a struct to encapsulate the crafting inventory cache

6ffd7e5

eltank force-pushed the fix_book_prof_mitigation branch from dbc8e65 to 6ffd7e5 Compare May 31, 2021 06:23

clean up one hack

9625353

I-am-Erk approved these changes Jun 3, 2021

View reviewed changes

mlangsdorf approved these changes Jun 3, 2021

View reviewed changes

Add TODO for getting rid of const_cast

6cb8ded

I-am-Erk merged commit 6ddf008 into CleverRaven:master Jun 4, 2021

eltank deleted the fix_book_prof_mitigation branch June 4, 2021 05:27

eltank mentioned this pull request Jun 6, 2021

Reduce crafting proficiency penalties based on partial proficiency level #49198

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix proficiency books' effect on crafting #48650

Fix proficiency books' effect on crafting #48650

eltank commented Apr 25, 2021 •

edited

eltank commented Apr 26, 2021

Salty-Panda commented Apr 26, 2021

eltank commented Apr 26, 2021

ferociousdork commented Apr 26, 2021

eltank commented Apr 27, 2021

eltank commented Apr 27, 2021 •

edited

eltank commented Apr 27, 2021

ghost commented May 27, 2021 •

edited by ghost

I-am-Erk commented May 28, 2021

actual-nh commented May 28, 2021

actual-nh commented May 28, 2021 •

edited

eltank commented May 28, 2021

eJ0opb commented May 29, 2021

eltank commented May 29, 2021

eltank commented May 30, 2021

jbytheway left a comment

jbytheway May 30, 2021

eltank May 31, 2021

I-am-Erk Jun 3, 2021

eltank Jun 4, 2021

eltank commented May 30, 2021 •

edited

actual-nh commented May 31, 2021

eltank commented May 31, 2021 •

edited

eltank commented May 31, 2021

I-am-Erk left a comment

I-am-Erk Jun 3, 2021

Fix proficiency books' effect on crafting #48650

Fix proficiency books' effect on crafting #48650

Conversation

eltank commented Apr 25, 2021 • edited

Summary

Purpose of change

Describe the solution

Describe alternatives you've considered

Testing

Additional context

eltank commented Apr 26, 2021

Salty-Panda commented Apr 26, 2021

eltank commented Apr 26, 2021

ferociousdork commented Apr 26, 2021

eltank commented Apr 27, 2021

eltank commented Apr 27, 2021 • edited

eltank commented Apr 27, 2021

ghost commented May 27, 2021 • edited by ghost

I-am-Erk commented May 28, 2021

actual-nh commented May 28, 2021

actual-nh commented May 28, 2021 • edited

eltank commented May 28, 2021

eJ0opb commented May 29, 2021

eltank commented May 29, 2021

eltank commented May 30, 2021

jbytheway left a comment

Choose a reason for hiding this comment

jbytheway May 30, 2021

Choose a reason for hiding this comment

eltank May 31, 2021

Choose a reason for hiding this comment

I-am-Erk Jun 3, 2021

Choose a reason for hiding this comment

eltank Jun 4, 2021

Choose a reason for hiding this comment

eltank commented May 30, 2021 • edited

actual-nh commented May 31, 2021

eltank commented May 31, 2021 • edited

eltank commented May 31, 2021

I-am-Erk left a comment

Choose a reason for hiding this comment

I-am-Erk Jun 3, 2021

Choose a reason for hiding this comment

eltank commented Apr 25, 2021 •

edited

eltank commented Apr 27, 2021 •

edited

ghost commented May 27, 2021 •

edited by ghost

actual-nh commented May 28, 2021 •

edited

eltank commented May 30, 2021 •

edited

eltank commented May 31, 2021 •

edited