-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 food monotony penalty from junk foods and psychoactives #38928
Conversation
src/item_factory.cpp
Outdated
is_not_boring = is_not_boring || mat == "junk"; | ||
} | ||
if (jo.has_member("material")) { | ||
for (auto& m : jo.get_tags("material")) { |
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.
Please replace auto with the proper type.
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.
What's the proper type? String? I didn't write that, just copied it from the existing material check.
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 like std::string.
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 didn't crash on item load, so that seems to be it. Thanks. Switched both of them.
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 would fail to compile if you put the wrong type, so you can know you've got it right.
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.
You need to astyle your changes.
actual types instead of auto astyle
src/item_factory.cpp
Outdated
} else if( jo.has_member( "material" ) ) { | ||
float specific_heat_solid = 0; | ||
float specific_heat_liquid = 0; | ||
float latent_heat = 0; | ||
|
||
for( auto &m : jo.get_tags( "material" ) ) { | ||
for( std::string m : jo.get_tags( "material" ) ) { |
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.
for( std::string m : jo.get_tags( "material" ) ) { | |
for( const std::string &m : jo.get_tags( "material" ) ) { |
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.
This doesn't work. I'm not actually sure why it did before, with auto.
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.
Try again, I was missing the const.
src/item_factory.cpp
Outdated
is_not_boring = is_not_boring || mat == "junk"; | ||
} | ||
if( jo.has_member( "material" ) ) { | ||
for( std::string m : jo.get_tags( "material" ) ) { |
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.
for( std::string m : jo.get_tags( "material" ) ) { | |
for( const std::string &m : jo.get_tags( "material" ) ) { |
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.
I don't follow the rationale that a wide variety of junkfoods aren't subject to the monotony effect.
@kevingranade I assumed it's because they're made of mostly fat and sugar. It was the intended behaviour back in #35910, you'd have to ask @Davi-DeGanne for his original reason. |
@Soadreqm Did you test that items with an explicitly-defined |
@wapcaplet No, I didn't. I actually don't think there are any. I don't think messing with the default should change how that works, but I guess I should try it out anyway. |
Set monotony_penalty of meat pie to 0 and meat pizza to 1, spawned and ate several pieces. It works as advertised. |
src/item_factory.cpp
Outdated
is_not_boring = is_not_boring || jo.get_int( "stim" ) != 0; | ||
} | ||
|
||
if( is_not_boring ) { // Junk food, stimulants and depressants never get old by default, but this can still be overriden. |
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.
Please out the comment on the line above or below, this can be hard to read.
After revisiting the parent issue, I see what you're doing now, part of the problem is you mixed a bugfix with a balance change, which made me think that it was all balance change. The bugfix part of this that prevents "primary_material" from making the code ignore "materials" can go in as-is, but please drop the stimulant part. Also I'm going to be reconsidering the junkfood exclusion in general in the future, because I'm very unconvinced it should be immune to this effect. |
Sure, I really should have separated the changes to begin with.
though? If I did a new PR for the stims, what would you actually expect it to have? |
By the way, I don't know why the automated tests are failing. It says it's the distribution of attacks to body parts, but I don't see how anything I've done could have messed with that. |
Test failures are due to randomness and #38983. |
Other failures you may have seen will be fixed by #38979 |
Summary
SUMMARY: Bugfixes "Removes food monotony penalty from junk foods"
Purpose of change
Fixes #38625
While I was at it, I also excluded caffeine and alcohol products from having the penalty, on the basis that taste doesn't matter as much for stimulants and depressants.Describe the solution
Moved the junk food check so that both "material" and "primary_material" get checked.
Added check for nonzero "stim" value, which covers all stimulants and depressants.Describe alternatives you've considered
Giving a monotony penalty to bee balm tea and chamomile tea. Removing penalty from herbal tea, pine needle tea, dandelion tea, brownies (both kinds) and semi-fancy foods with small portion sizes.
Since the monotony penalty is applied per portion, foods with high charges are affected more. The various pies are the worst right now, coming in stacks of 8 and losing all flavor by the third slice. That's something that should be considered per-item, though.
Brownies are interestingly enough not junk food, despite having chocolate, and don't have a stim effect, despite having marijuana, so they still get boring.
The non-tea teas are kind of borderline. I don't think any of them have any caffeine in reality, but in-game bee balm tea has 1 stim, chamomile is copied from regular tea, and the rest are just mildly flavored water.
Testing
Spawned chocolate waffles, chocolate drink, regular chocolate, cola, coffee, tea, vodka and lasagne. Chocolate products,
caffeine and boozedid not get boring when eaten, while lasagne did.Searched through json files in data/items/comestibles for "stim" to see if anything weird was using it. Noted that "alien fronds" and "leech flowers" act as stimulants, but that seems fairly reasonable considering the item descriptions.Spawned alien fronds and leech flowers, which did not get boring when eaten.