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

Remove bionic mana penalty #2404

Merged
merged 2 commits into from
Mar 11, 2023

Conversation

chaosvolt
Copy link
Member

@chaosvolt chaosvolt commented Mar 11, 2023

Summary

SUMMARY: Balance "Bionic power no longer affects maximum mana"

Purpose of change

Per discussion on the BN discord, implementing an in-repo example of mana-casting that isn't affected by the bionic energy penalty was running into delays (as the main example of it in action was planned to be Arcana mod, when it's ported over to become an in-repo mod), and most opinions have weighed in favor of it being fine to just axe the feature outright.

Describe the solution

  1. In magic.cpp, updated known_magic::max_mana to remove the bp_penalty feature entirely.
  2. Updated enchantment_test.cpp, removing test cases and data only relevant to testing if the now-removed penalty is working correctly.

Describe alternatives you've considered

  1. Rigging a quick-and-dirty in-repo solution to allow [DDA Port] Add mutation value that lessens bionic power penalty on mana cap #1823 to be merged without any more delays.
  2. Moving forward with porting over Arcana anyway and just figuring out what the merge order should be to get it all to come together without causing any problems.

Testing

  1. Compiled and load-tested.
  2. Spawned in a character and debugged in all spells.
  3. Observed max mana was default of 1000.
  4. Debugged in some filled power storage.
  5. Waited a bit and confirmed man mana didn't change.
  6. Checked affected file for astyle.

Additional context

@github-actions github-actions bot added the src changes related to source code. label Mar 11, 2023
@olanti-p
Copy link
Contributor

There are tests that check that the penalty exists & works, it has to be remove from there as well

-------------------------------------------------------------------------------
Mana pool
-------------------------------------------------------------------------------
enchantment_test.cpp:551
...............................................................................

enchantment_test.cpp:511: FAILED:
  REQUIRE( guy.magic->max_mana( guy ) == t.norm_cap )
with expansion:
  1000 (0x3e8) == 750 (0x2ee)
with message:
  t.idx := 2

-------------------------------------------------------------------------------
Mana pool
-------------------------------------------------------------------------------
enchantment_test.cpp:551
...............................................................................

enchantment_test.cpp:511: FAILED:
  REQUIRE( guy.magic->max_mana( guy ) == t.norm_cap )
with expansion:
  1,000 (0x3e8) == 750 (0x2ee)
with message:
  t.idx := 2

@github-actions github-actions bot added the tests changes related to tests label Mar 11, 2023
@chaosvolt
Copy link
Member Author

Was just now working on that, yes.

@olanti-p olanti-p merged commit 7e5d571 into cataclysmbnteam:upload Mar 11, 2023
@chaosvolt chaosvolt deleted the third-times-the-charm branch March 11, 2023 19:38
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. tests changes related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants