-
Notifications
You must be signed in to change notification settings - Fork 288
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
feat: JSONize spellcasting encumbrance threshold #6067
feat: JSONize spellcasting encumbrance threshold #6067
Conversation
base_leg_encumbrance value when calculating how much encumbrance is ignored for casting a spell instead of the previous hard-coded 20. The JSON value is optional, and it defaults to 20.
Autofix has formatted code style violation in this PR. I edit commits locally (e.g: git, github desktop) and want to keep autofix
I do not want the automated commit
If you don't do this, your following commits will be based on the old commit, and cause MERGE CONFLICT. |
Code looks good to me, now to compile and test |
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.
- Compiled and load-tested.
- Loaded in a Magical Nights save as a novice stormshaper.
- Observed my failure rate with Windrunning does not go up any when equipping a riot shield (10 enc on only a single arm) nor even a banded shield (20 enc on one arm).
- Observed that failure rate starts to go up if I put on arm warmers over said banded shield, increasing total enc from 20 to 22 (1 per arm).
I was once again almost bamboozled by the function being goofy and counting the encumbrance of each limb independently.
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.
Looks good! Compiled, and made sure that it works on my machine properly. Even tested setting the threshold manually above the default 20 and it worked.
Only suggestion is just related to the discussion in the Discord about making the fields more sensibly named
src/magic.cpp
Outdated
@@ -317,6 +317,8 @@ void spell_type::load( const JsonObject &jo, const std::string & ) | |||
optional( jo, was_loaded, "base_energy_cost", base_energy_cost, 0 ); | |||
optional( jo, was_loaded, "final_energy_cost", final_energy_cost, base_energy_cost ); | |||
optional( jo, was_loaded, "energy_increment", energy_increment, 0.0f ); | |||
optional( jo, was_loaded, "base_arm_encumbrance_value", base_arm_encumbrance_value, 20 ); |
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.
optional( jo, was_loaded, "base_arm_encumbrance_value", base_arm_encumbrance_value, 20 ); | |
optional( jo, was_loaded, "arm_encum_thresh", arm_encumbrance_threshold, 20 ); |
Perhaps this would be a better set of names? And similar can go for the leg values too
…umbrance_minimum_value
If anything we could even unify it to use just one value for all properties but, which would OP prefer I guess? |
arm_encumbrance_threshold and leg_encumbrance_threshold
Pursuant to feedback, I changed the variable and JSON names to |
Personally, I'm confused. Arm encumbrance is OPT-IN (it won't count unless the |
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.
Whoops, didn't see the reply to this. Seems fine then.
base_leg_encumbrance value when calculating how much encumbrance is ignored for casting a spell instead of the previous hard-coded 20. The JSON value is optional, and it defaults to 20.
Purpose of change (The Why)
Casting a spell using encumbrance ignored the first
20
points of arm and leg encumbrance in magic.cpp.20
is a hard-coded value.Describe the solution (The How)
The values are now optional JSON fields (named
base_arm_encumbrance_value
andbase_leg_encumbrance_value
) per spell with a default value of20
. No documentation was added for the field.Describe alternatives you've considered
Testing
I verified the fields are accepted when input as values for spells in JSON.
Checklist
Mandatory
main
so it won't cause conflict when updatingmain
branch later.