-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Rewrite crafting skill checks to use normal roll #63886
Merged
dseguin
merged 21 commits into
CleverRaven:master
from
anothersimulacrum:erks-success-rolls
Mar 9, 2023
Merged
Rewrite crafting skill checks to use normal roll #63886
dseguin
merged 21 commits into
CleverRaven:master
from
anothersimulacrum:erks-success-rolls
Mar 9, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: actual-nh <74678550+actual-nh@users.noreply.github.com>
Make intelligence neutral at an average of 8. make final_difficulty tallied the same as skill total fix a dumb gaffe from me
just some comments as I review. begone, unused variable. Co-authored-by: actual-nh <74678550+actual-nh@users.noreply.github.com> Co-authored-by: anothersimulacrum <anothersimulacrum@gmail.com>
can be worked to use morale later, not important at the moment.
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.
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.
Add a number of benchmark recipes which need only a 2x4 for tools/ingredients, and test that their actual failure rates match the calculated one and pre-provided ones at various levels of skill and proficiency. Also add a synthetic test to output the calculated chances for every recipe in the game.
anothersimulacrum
force-pushed
the
erks-success-rolls
branch
from
March 7, 2023 16:39
1c88fc9
to
b4c7879
Compare
github-actions
bot
added
[C++]
Changes (can be) made in C++. Previously named `Code`
[JSON]
Changes (can be) made in JSON
Code: Tests
Measurement, self-control, statistics, balancing.
Crafting / Construction / Recipes
Includes: Uncrafting / Disassembling
Info / User Interface
Game - player communication, menus, etc.
<Bugfix>
This is a fix for a bug (or closes open issue)
astyled
astyled PR, label is assigned by github actions
json-styled
JSON lint passed, label assigned by github actions
labels
Mar 7, 2023
github-actions
bot
added
the
BasicBuildPassed
This PR builds correctly, label assigned by github actions
label
Mar 8, 2023
thank you for this |
we've done it Sim, we've destroyed one of the core parts of the game. Bwahahaha. |
andrei8l
reviewed
Mar 20, 2023
float Character::crafting_success_roll( const recipe &making ) const | ||
{ | ||
craft_roll_data data = recipe_success_roll_data( making ); | ||
float craft_roll = std::max( normal_roll( data.center, data.stddev ), 0.0 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get an assertion failure here
/usr/include/c++/12.2.1/bits/random.h:1995: std::normal_distribution<_RealType>::param_type::param_type(_RealType, _RealType) [with _RealType = double]: Assertion '_M_stddev > _RealType(0)' failed.
Build with -Wp,-D_GLIBCXX_ASSERTIONS
and run cata_test proficiency_gain_short_crafts
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: Tests
Measurement, self-control, statistics, balancing.
Crafting / Construction / Recipes
Includes: Uncrafting / Disassembling
Info / User Interface
Game - player communication, menus, etc.
[JSON]
Changes (can be) made in JSON
json-styled
JSON lint passed, label assigned by github actions
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
SUMMARY: Features "Tweak crafting failure formulas and display failure chances"
Purpose of change
See #46153 for maximal detail, this replaces that.
The goals of this are two-fold:
There are also longer-term goals such as #44738, and enabling partial skill to play a role.
Describe the solution
From @I-am-Erk 's rewrite of the formula:
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).
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.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.
From my additional changes:
Calculate and display the failure chance for a recipe 9f2ee4d
Adjust the chance for destruction of components to follow a different formula 58f5676, and display that chance (catastrophic failure chance). This is just an easier version of the above formula for now, I just fudged it to get pretty numbers.
Add some tests to:
Testing
See tests.
(Ridiculous percentages due to debug set skills command)
Additional context
This has ping-ponged between Erk and I for a while, and the commit history reflects that. Mergers may want to avoid squashing for history reasons.
I do not expect this to be the final formula(s), this just puts the infrastructure in place.
The proficiency info screen is lying about how the failure rates work from proficiencies. I want to fix that (included in https://github.com/CleverRaven/Cataclysm-DDA/pull/46082/commits), but it's a lot of lines, and so I think it's better separate.
Closes #46153