-
Notifications
You must be signed in to change notification settings - Fork 290
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
Port over mutations granting effects (and fix) #180
Conversation
Running character.cpp through Astyle 3.1 shows a lot of changes the style edit proposes, but nothing in the sections this PR touched, far as I can tell? Unless the message is saying game_inventory.cpp is where the astyle failure is, which would be odd given that wasn't touched at all. |
That's my fault, thanks for reporting. |
{ | ||
"id": "debug_active_relic_test", | ||
"type": "TOOL", | ||
"name": { "str": "Cube of Shame", "str_pl": "Cubes of Shame" }, |
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.
src/translations.cpp:530 [translation::deserialize(JsonIn&)::<lambda(const string&, int)>]
line 203:40: Please use "str_sp" instead of "str" and "str_pl" for text with identical singular and plural forms
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.
Plural string is "cubes of shame" which is not identical?
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.
It's for line 203 https://github.com/cataclysmbnteam/Cataclysm-BN/pull/180/files#diff-719e962e4193c09bda9e0a2f71d6946610bf808ed76e341be1cd18cbb4f6bbc8R203
Edit: nvm, you've found it
"magazine_well": 1, | ||
"relic_data": { | ||
"passive_effects": [ | ||
{ "has": "HELD", "condition": "ALWAYS", "mutations": [ "SCHIZOPHRENIC" ] }, |
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.
SCHIZOPHRENIC
is a terrible mutation for testing because it's RNG-dependent.
Try something like carnivore.
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.
True, was a bit of waiting. Had picked it more for thematic purposes, but can change to Carnivore next time I alter its properties.
"color": "blue", | ||
"weight": "400 g", | ||
"volume": "500 ml", | ||
"charges_per_use": 1, |
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.
It doesn't seem to be introduced here, but there is a bug with the system: charges_per_use
do nothing here.
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.
Artifact from having based the item off a standard electronic item (fairly certain it was a flashlight), a lot of transforming items in the game have a charges per use but don't seem to respect it, seen in both DDA and BN.
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.
Oh right: charge consumption occurs at the end of the function, but in transformation, this would consume charges from the transformed item, which may not work.
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.
That could be it. Placing it earlier in the order would fix that, but it might be good to ensure that the check for needs_charges
still happens earlier. Otherwise, presumably an item that checks for having a certain number of charges would (if it's exactly at the threshold for passing the needs_charges
check) consume its charges, then fail the check.
Crap, I accidentally merged it before testing it. |
Ah, I just now found the translation error. It's in the name of the active version, not the inactive one. |
Summary
SUMMARY: Features "Port over changes allowing enchantments to grant mutations, more flexible implementation of relic-based clairvoyance."
Purpose of change
Adds the code changes from the PRs that allow for enchantments to grant the effects of mutations. This includes the clairvoyance update that was added at the same time as the first PR, and the code changes in the followup PR so that it wasn't pretty much only applying mutation flag effects.
However, instead of tying clairvoyance to a mutation flag, I implemented it via effect flag instead. This is more versatile, not only can any source of enchantments thus provide clairvoyance via granting an effect, but likewise any source of effects (not just enchantments) can do so.
This will likely go well with future plans to JSONize more hardcoded checks to allow using effect flags.
Describe the solution
Describe alternatives you've considered
Adding in the stranger method of checking for clairvoyance via mutation flag anyway. I was intially tempted to just add both to the code since it wouldn't really hurt, but I realized that using an effect flag for it can ALREADY do everything that the mutation flag can do, making the mutation flag method redundant.
Testing
Testing:
Additional context
Relevant PRs:
Not ported over:
Feel free to request any mutation checks that might be desirable to convert into an effect-flag check. Reasonably easy for me to do.