-
Notifications
You must be signed in to change notification settings - Fork 288
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(balance): Allow for use of multiple sleep aids #5848
feat(balance): Allow for use of multiple sleep aids #5848
Conversation
And print them out too!
Autofix has formatted code style violation in this PR. I edit commits locally (e.g: git, github desktop) and want to keep autofix
I do not want the automated commit
If you don't do this, your following commits will be based on the old commit, and cause MERGE CONFLICT. |
Huh, I accidentally made use of curly brace-less for loop. That's funny, good thing the tree caught it though |
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.
code looks alright but needs playtesting
thanks scarf, very cool Co-authored-by: scarf <[email protected]>
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.
also found some more nitpicks but that doesn't stop the PR from shipping
@@ -243,12 +244,11 @@ comfort_response_t base_comfort_value( const Character &who, const tripoint &p ) | |||
const std::optional<vpart_reference> board = vp.part_with_feature( "BOARDABLE", true ); | |||
if( carg ) { | |||
const vehicle_stack items = vp->vehicle().get_items( carg->part_index() ); | |||
for( const item *items_it : items ) { | |||
for( item *items_it : items ) { |
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.
does the const
modifier here prevent putting it in comfort_info.aid
? might go extra careful and could copy them (const
good, mutability bad)
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.
Yes, I had to remove the const modifier here to get rid of an error in the IDE
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.
In particular, push_back
complains when trying to put it into the vector
@@ -278,14 +278,13 @@ comfort_response_t base_comfort_value( const Character &who, const tripoint &p ) | |||
comfort -= here.move_cost( p ); | |||
} | |||
|
|||
if( comfort_response.aid == nullptr ) { | |||
if( comfort_response.aid.empty() ) { |
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 if condition prevents player from getting aid from both vehicle and floor, is this intended behavior?
e.g below a car on floor is teddy bear, the car also has teddy bear -> only 1 teddy bear bonus (car)
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.
Not intentional in the sense that I had even thought of it, but I'd say it's sensible that only the item that's in the actual vehicle should count.
initialization should handle it
After applying the suggestion to remove the unncecessary vector clearing (and the other pertinent small changes and suggestions), can confirm it still builds and works as expected on my machine |
Hopefully, main thing however is that if you're bringing pillows from multiple houses one at a time, the stack-splitting might kick in and ensure every single one you add gets its own message line. That'll add up and will affect even a normal playthrough, just just someone who's hoardmaxxing. |
Hmm. |
Fair, it hopefully won't be too cursed. |
also wondering: does stacks work, e g is 40x teddy bear as effective as 1x teddy bear |
... probably depends on if it's count / charges or individual items |
It just iterates over the items on the tile, so it should work just fine if they're individual items? |
In any case, looks like it behaves just fine. As such, |
Checklist
Required
main
so it won't cause conflict when updatingmain
branch later.Purpose of change
As expressed by many, not allowing the survivor to use a pillow and a teddy bear at the same time is ridiculous. People use multiple pillows, cuddle a stuffed animal or body pillow while using a normal pillow, etc. As such, this is clearly a highly-desired change and is in-fact more realistic than how it was before.
Also, if any balance hinges on only one sleep aid being allowed, we have bigger problems.
Describe the solution
break
lines that prevent multiple sleep aids from being usedcomfort_response.aid
into a vector instead of an item pointerDescribe alternatives you've considered
Doesn't adequately account for the use cases.
Testing
It builds and it works on my machine
![image](https://private-user-images.githubusercontent.com/30732426/399101123-20d5586c-62e4-4ad9-9b2f-736b1cb532c0.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk5MzM0OTEsIm5iZiI6MTczOTkzMzE5MSwicGF0aCI6Ii8zMDczMjQyNi8zOTkxMDExMjMtMjBkNTU4NmMtNjJlNC00YWQ5LTliMmYtNzM2YjFjYjUzMmMwLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMTklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjE5VDAyNDYzMVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTNlNzJmOTBhZjY4MGQ2ZTliMjcwODY0YTlmMDViNjE2ZmM0NDc3NjBjNWY0ZmY1NjQ3NjlhYzIzZmJlY2M2YTQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.klOtXfD_6a4RN7c64OD_Xp-AyNcTT0AC5FvsQeM1mR0)
Additional context
I hope my conversion of
comfort_response.aid
to a vector isn't bad code.Can't wait for the pillow-stacking meta of sleep comfort