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: Spell damage types case-insensitive, begin migration from none to true #5916

Merged
merged 5 commits into from
Jan 12, 2025

Conversation

RobbieNeko
Copy link
Collaborator

Checklist

Required

Optional

  • This is a C++ PR that modifies JSON loading or behavior.
    • I have documented the changes in the appropriate location in the doc/ folder.
    • If applicable, add checks on game load that would validate the loaded data.

Purpose of change

Case sensitivity is annoying, especially when it was already inconsistently applied to begin with. Additionally, I recently found out that we were using none as the word for true damage, which is absurd given that DT_NONE is a real damage type that should error.
Additionally, any unrecognized damage type string defaulting to true damage is absurd.

Describe the solution

  • Uppercases the incoming string so that case doesn't matter (at the cost of losing const on the string)
  • Adds TRUE to the if-else list as the correct method of getting true damage
  • Adds a debug message when it detects None, but still treats it as true damage to give ample time for modders to get their ducks in a row
  • Changes the damage type given from an unrecognized string to bash since if they're specifying a damage type they probably don't want True damage (and true damage is very powerful, whereas bash has actual resistance)
  • Updates docs to better reflect current status
  • Bring Magical Nights in line with the changes

Describe alternatives you've considered

  • Wait for someone else to do it
  • Only let full lowercase and full uppercase damage strings work instead of uppercasing the string
  • Leave the true damage behavior for unrecognized entries be
  • Change the default when not specifying to something less powerful than true

If they're not specifying the damage type, I think it's reasonable to presume they want the truest form of damage that does exactly what they tell it to.

Testing

It builds, it loads, the casing works, and I can even do bullet damage

Additional context

Proper removal of none from the function (or harder load-errors) can probably occur in a few months

none gets hit with a debug message nagging you but still works FOR NOW
@github-actions github-actions bot added docs PRs releated to docs page src changes related to source code. JSON related to game datas in JSON format. mods PR changes related to mods. labels Jan 10, 2025
Copy link
Contributor

autofix-ci bot commented Jan 10, 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 Author

Oh of course tests decide to test a spell with the none damage type

@github-actions github-actions bot added the tests changes related to tests label Jan 10, 2025
@AskaHope
Copy link

I can even do bullet damage

I cast bullet!

@chaosvolt chaosvolt merged commit 5fe8458 into cataclysmbnteam:main Jan 12, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs PRs releated to docs page JSON related to game datas in JSON format. mods PR changes related to mods. src changes related to source code. tests changes related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants