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

Needs testing: Rewrite crafting skill checks to use normal_roll. #46153

Closed

Conversation

I-am-Erk
Copy link
Member

@I-am-Erk I-am-Erk commented Dec 19, 2020

Summary

SUMMARY: Infrastructure "Rewrite crafting skill checks to use normal_roll"

Purpose of change

I have revived this PR after a few years inactive, because we still need it and it turns out it wasn't that stale.

1. Proficiency failure penalties are currently not being applied correctly. This is indirectly because the way skill checks are calculated is needlessly complicated. no longer relevant.

  1. Per MAJOR DISCUSSION. Standardize all random checks to a single json object and set of function calls #44738, it is a good idea to start moving major skill checks into normal rolls. Then when we create a standardized roll_check routine, it will be much easier to write and to migrate. The current crafting skill check method is impenetrably complex and the complexity offers no real benefit.
  2. Per eoc_test: fix setup for EOC_recipe_test #66064, there are two crafting check issues that need resolution: first, crafting checks from skill 0 always fail, and second, crafting checks do not take into account marginal skill gain which interacts poorly with rust mechanics and will cause problems when skill gain is slowed.

Describe the solution

To solve both of these, I have migrated the crafting skill check to use normal_roll and make it mathematically predictable. This brings in a few major changes:

  1. NPCs no longer have to be more skilled than you to increase your chance of success. Instead, each NPC able to help that also has all the prerequisites to be able to do the craft on their own gets a skill check just like you. Then we add those skill checks together using the formula:
final skill value = sqrt( your skill value^2 + sum (all contributing NPC skill values^2) )

This root-sum-of-squares is the same system used in summing morale: it creates diminishing returns the more contributors you have, and makes more skilled sources of help substantially outweigh smaller ones. Using this formula, for example, if you and a helper NPC both have an effective skill level of 7 once all effects are applied, your combined skill level is around 10. If you add another NPC of effective skill level 7, you get a total combined skill of 12.

In the end this makes NPC helpers much more useful (previously they just added a small flat bonus, and only if they were more skilled than you).

  1. Changes the role of secondary and primary skills in your check, making secondary skills potentially more important. Previously the weight of primary and secondary skill requirements was flat: 3/4 of your skill check was from your primary skill and 1/4 was from the average of the secondary skills. Now it is a weighted average of: 2*primary skill*required level of primary skill, + sum of secondary skills * required level of secondary skills. That means if a recipe asks for electronics 2 and cooking 1 (what the hell recipe is this??), your overall level in electronics will be twice as important to your final skill value. If that same recipe uses fabrication 2 as its primary skill, then fabrication will weigh in at 2x as important as electronics and 4x as important as cooking.
  2. Moves the method of calculating your effective skill level to a new method to make it easier to determine your and your NPCs' effective skills with all modifiers included.
  3. (not done) Includes your marginal skill gain in the calculation, so having skill level 1(50%) is equal to having skill level 1.5, and progress in a skill matters.

Describe alternatives you've considered

I would like some more features in a follow up PR.

  • I want to jsonize what role attributes have in skills. Currently only intelligence matters. I'd suggest the simplest way would be to define defaults in the skills in skills.json themselves, and add some optional fields for recipes that could override these defaults at some later date, similar to how we do it for proficiencies where there's a default and an override. So for example, electronics recipes could be intelligence based by default, but often use perception instead.
    • If we decide to move away from using attributes at all in the next stable, this should instead use the surrogates we develop for attribute roles.
  • I want to jsonize mutations that affect specific skills. Since I first did this PR the mutations themselves have been jsonized, but I would like the skill penalty to be in JSON, either as part of the mutation or part of the skill.
  • I would very much like to display chance to craft in the crafting UI, but I doubt I have the skill to do that.

Testing

Compiles, loads, and allows crafting. Crafting skill rolls appear to be happening correctly but do have too wide of a range in my opinion. Failures seem to be a bit difficult still, but I would like to test more and I would like some help with that.

image

As you can see this has my second roll on an effective skill of 3 as a 10. That might just be an outlier but it's pretty sus, I think the standard deviation widening at lower levels may be a mistake, or perhaps that should go along with also dropping the roll a bit.

Additional context

It's kind of insane that this PR loaded and compiled with minimal edits after 2 years.

src/character.h Outdated Show resolved Hide resolved
Co-authored-by: actual-nh <74678550+actual-nh@users.noreply.github.com>
@I-am-Erk I-am-Erk added <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Crafting / Construction / Recipes Includes: Uncrafting / Disassembling labels Dec 19, 2020
@I-am-Erk I-am-Erk added this to In progress in 0.F Release Planning via automation Dec 19, 2020
@I-am-Erk I-am-Erk added this to In progress in Overhaul randomization and actions via automation Dec 19, 2020
src/crafting.cpp Outdated Show resolved Hide resolved
src/crafting.cpp Outdated Show resolved Hide resolved
src/crafting.cpp Outdated Show resolved Hide resolved
I-am-Erk and others added 3 commits December 30, 2020 22:48
Co-authored-by: actual-nh <74678550+actual-nh@users.noreply.github.com>
Co-authored-by: anothersimulacrum <anothersimulacrum@gmail.com>
Co-authored-by: actual-nh <74678550+actual-nh@users.noreply.github.com>
@I-am-Erk I-am-Erk changed the title WIP: Rewrite crafting skill checks to use normal_roll. Needs testing: Rewrite crafting skill checks to use normal_roll. Aug 12, 2022
@I-am-Erk I-am-Erk marked this pull request as ready for review August 12, 2022 03:43
@I-am-Erk
Copy link
Member Author

I-am-Erk commented Aug 12, 2022

Shockingly and against all odds this is now at a place where people could help me playtest. It is not yet ready to merge, see my own above unresolved comments

@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label Aug 12, 2022
src/crafting.cpp Outdated Show resolved Hide resolved
src/crafting.cpp Outdated Show resolved Hide resolved
I-am-Erk and others added 2 commits September 26, 2022 10:49
* Display recipe chance of failure

Split the crafting roll function into just providing the parameters for
the normal curve it will be rolling on, and a function doing the actual
roll.
Also, fix some DBZ bugs, and add debug outputs while we're there.

Then, add a function to calculate the chance we'll pass a normal roll
with given center, stddev, and difficulty, and use that to provide the
recipe success chance.
Add this to the crafting GUI.

* Adjust crafting catastrophic failure chances

Fudge the numbers to make catastrophic (item-destroying) failures much
less likely than setback failures.
Also display the chance of a catastrophic failure in the UI.
@github-actions github-actions bot added the Info / User Interface Game - player communication, menus, etc. label Oct 8, 2022
@anothersimulacrum anothersimulacrum self-assigned this Oct 10, 2022
@anothersimulacrum anothersimulacrum removed their request for review October 10, 2022 17:39
@Night-Pryanik
Copy link
Contributor

@I-am-Erk could you please resolve conflicts?

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Oct 15, 2022
Fris0uman pushed a commit to kevingranade/Cataclysm-DDA that referenced this pull request Nov 19, 2022
Co-authored-by: I-am-Erk <I-am-Erk@users.noreply.github.com>
Fris0uman pushed a commit to Fris0uman/Cataclysm-DDA that referenced this pull request Jan 14, 2023
Co-authored-by: I-am-Erk <I-am-Erk@users.noreply.github.com>
Overhaul randomization and actions automation moved this from In progress to Done Mar 9, 2023
@I-am-Erk I-am-Erk deleted the change-skill-success-rolls branch March 9, 2023 18:09
Fris0uman pushed a commit to Fris0uman/Cataclysm-DDA that referenced this pull request Mar 13, 2023
Co-authored-by: I-am-Erk <I-am-Erk@users.noreply.github.com>
Fris0uman pushed a commit to Fris0uman/Cataclysm-DDA that referenced this pull request Mar 26, 2023
Co-authored-by: I-am-Erk <I-am-Erk@users.noreply.github.com>
Fris0uman pushed a commit to Fris0uman/Cataclysm-DDA that referenced this pull request May 9, 2023
Co-authored-by: I-am-Erk <I-am-Erk@users.noreply.github.com>
Fris0uman pushed a commit to Fris0uman/Cataclysm-DDA that referenced this pull request May 21, 2023
Co-authored-by: I-am-Erk <I-am-Erk@users.noreply.github.com>
Fris0uman pushed a commit to Fris0uman/Cataclysm-DDA that referenced this pull request May 28, 2023
Co-authored-by: I-am-Erk <I-am-Erk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Crafting / Construction / Recipes Includes: Uncrafting / Disassembling Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions
Projects
Abandoned but Desired PRs
Abandoned PRs that someone is trying ...
0.F Release Planning
  
Delayed blockers (for 0.G)
Development

Successfully merging this pull request may close these issues.

None yet

9 participants