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

feat: JSONize spellcasting encumbrance threshold #6067

Conversation

Hourousha
Copy link
Contributor

@Hourousha Hourousha commented Feb 8, 2025

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 and base_leg_encumbrance_value) per spell with a default value of 20. 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

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.
@github-actions github-actions bot added the src changes related to source code. label Feb 8, 2025
Copy link
Contributor

autofix-ci bot commented Feb 8, 2025

Autofix has formatted code style violation in this PR.

I edit commits locally (e.g: git, github desktop) and want to keep autofix
  1. Run git pull. this will merge the automated commit into your local copy of the PR branch.
  2. Continue working.
I do not want the automated commit
  1. Format your code locally, then commit it.
  2. Run git push --force to force push your branch. This will overwrite the automated commit on remote with your local one.
  3. Continue working.

If you don't do this, your following commits will be based on the old commit, and cause MERGE CONFLICT.

@RobbieNeko
Copy link
Collaborator

Code looks good to me, now to compile and test

@scarf005 scarf005 changed the title feat: Modified spells to use a JSON supplied value of encumbrance to ignore when casting a spell instead of the hard-coded value 20. feat: JSONize spellcasting encumbrance threshold Feb 9, 2025
chaosvolt
chaosvolt previously approved these changes Feb 9, 2025
Copy link
Member

@chaosvolt chaosvolt left a comment

Choose a reason for hiding this comment

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

  1. Compiled and load-tested.
  2. Loaded in a Magical Nights save as a novice stormshaper.
  3. 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).
  4. 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.

Copy link
Collaborator

@RobbieNeko RobbieNeko left a 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 );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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

@chaosvolt
Copy link
Member

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
@Hourousha
Copy link
Contributor Author

Pursuant to feedback, I changed the variable and JSON names to arm_encumbrance_threshold and leg_encumbrance_threshold. I searched the code and encum is less frequent in the code base than encumbrance. Honestly, my preference is just using one (either encum or encumbrance) everywhere because I personally can't remember which to use where when there's more than one (sometimes, I still struggle when there's just one). I completely defer to you as I can't even grok a lot of the C++ code base.

@Hourousha
Copy link
Contributor Author

If anything we could even unify it to use just one value for all properties but, which would OP prefer I guess?

Personally, I'm confused. Arm encumbrance is OPT-IN (it won't count unless the spell_flags JSON SOMATIC is set, but leg encumbrance is OPT-OUT (it's will count unless the spell_flags JSON NO_LEGS is set), so I'm completely confused about unifying the values.

Copy link
Member

@chaosvolt chaosvolt left a 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.

@chaosvolt chaosvolt merged commit 5745561 into cataclysmbnteam:main Feb 18, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
src changes related to source code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants