Skip to content

/Cataclysm-DDA

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

[RDY] Initial craft entity implementation #28937

Merged
merged 7 commits into from Apr 9, 2019

Conversation

Reviewers

@codemime codemime

@kevingranade kevingranade

Assignees
No one assigned
Labels
<Bugfix> Character / Player Crafting / Construction / Recipes Game: Mechanics Change Items / Item Actions / Item Qualities [C++]
Projects
  
Done in 0.E Release
  
Done in Tailoring Overhaul
Milestone
No milestone
5 participants
@ifreund @int-ua @codemime @Night-Pryanik @kevingranade
@ifreund
Copy link
Contributor

commented Mar 23, 2019 •
edited

Summary

SUMMARY: Features "Represent in progress crafts as an item."

Purpose of change

First step in #29210
Closes #28775
Closes #25188
Fixes #28718
Fixes #28930
Closes #28698
Closes #28592
Fixes #26264
Closes #23971
Closes #21850
Fixes #18191

Describe the solution

  • Create a new item craft
  • Create a new funtion start_craft() to generate the item
  • Create a use action to work on the craft
  • Modify the craft activity to use the new item
  • Store the components in the craft item
  • Weight/volume of the craft item is the sum of the components
  • Reimplement long crafting
  • Fix/reimplement end of craft checks (waste materials/exp gain/rot)
  • Investigate the craft item inheriting the materials of its components
  • Fix item::type_name() (probably put "in progress craft" in the json and move the substitution string to the c++ code)
  • Improve craft item cosmetics (optional)
  • Search for bugs
  • Clean up code

Additional context

The main goal of this PR is to implement a fully functional craft item while keeping crafting mechanics mostly unchanged. It may prove easier to modify some checks to be incremental, but I really just want to get a solid foundation established that I can build on in subsequent pull requests.

@ifreund ifreund added Game: Mechanics Change Crafting / Construction / Recipes [C++] Items / Item Actions / Item Qualities <Bugfix> Character / Player labels Mar 23, 2019

@int-ua

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Mar 23, 2019

Related suggestion to lessen spoilage of ingredients (especially for large batches): #28930
@ifreund

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Mar 24, 2019 •
edited

Like I said, I'm not aiming to change mechanics too much in this pr. General discussion of the crafting overhaul belongs in #29210

@ifreund ifreund marked this pull request as ready for review Apr 1, 2019

@ifreund

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Apr 1, 2019

Whoops, wasn't quite ready to mark this as ready yet, still want to do a bit of code cleanup and it looks like I need to rebase as well.

@ifreund ifreund force-pushed the ifreund:craft-entity branch 2 times, most recently from c5336f6 to e5d845f Apr 1, 2019

@ifreund

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Apr 1, 2019

Ok this is ready for review. I had initially planned to try and make in progress crafts look like their products, but that can wait for one of the many planned follow up prs addressing #28775

@ifreund ifreund referenced this pull request Apr 1, 2019

Open

Crafting mechanics overhaul #29210

10 of 20 tasks complete

@ifreund ifreund added this to Confirmed in 0.E Release via automation Apr 1, 2019

@ifreund ifreund added this to In progress in Tailoring Overhaul via automation Apr 1, 2019

@codemime

This comment has been minimized.

Sign in to view
Copy link
Member

commented Apr 1, 2019

I don't see spoilage being handled anywhere in you code. There's a possible exploit related to it:

  1. Start crafting food from perishable components.
  2. Get crafting interrupted to acquire an in-progress craft.
  3. Enjoy your imperishable investment.

My point is, a craft made of perishable items should also be perishable, and it shouldn't last longer than its first to go off ingredient.
@ifreund

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Apr 1, 2019

Yeah, that's part of my todo list in #29210. I thought that the poor handling would be ok for now, but you're right that it is a little too exploitable to be merged in it's current state. Will look into a proper fix.
@ifreund

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Apr 2, 2019

Ok, components now rot while in an in progress craft. The relative rot of the most rotten component at completion is transferred to the product. This may be a little unrealistic in some preservation recipes, but that's a subject for a different pr, this one is large enough.
@codemime

codemime reviewed Apr 2, 2019
View changes

Show resolved Hide resolved src/activity_handlers.cpp Outdated
@codemime

codemime reviewed Apr 2, 2019
View changes

Show resolved Hide resolved src/activity_handlers.cpp Outdated
@codemime

codemime reviewed Apr 2, 2019
View changes

Show resolved Hide resolved src/activity_handlers.cpp Outdated
@codemime

codemime reviewed Apr 2, 2019
View changes

Show resolved Hide resolved src/crafting.cpp Outdated
@codemime

codemime reviewed Apr 2, 2019
View changes

Show resolved Hide resolved src/crafting.cpp Outdated
@codemime

codemime reviewed Apr 2, 2019
View changes

Show resolved Hide resolved src/item.cpp Outdated
@codemime

codemime reviewed Apr 2, 2019
View changes

Show resolved Hide resolved src/item.cpp
@codemime

codemime reviewed Apr 2, 2019
View changes

src/item.cpp

for( const item &it : components ) {
if( it.has_flag( "HIDDEN_POISON" ) ) {
set_flag( "HIDDEN_POISON" );

This comment has been minimized.

Sign in to view
Copy link
@codemime

codemime Apr 2, 2019

Member

This approach isn't particularly robust: imagine adding new flags or removing them. Essentially this snipped is code duplication with all its drawbacks. I'd tweak item::has_flag instead.

This comment has been minimized.

Sign in to view
Copy link
@ifreund

ifreund Apr 2, 2019

Author Contributor

The reason I did it this way is that I wasn't sure if it made sense for all flags to be transferred and didn't want to cause any potential strange behavior. Will take another look at this.
@codemime

codemime reviewed Apr 2, 2019
View changes

Show resolved Hide resolved src/item.cpp Outdated
@codemime

codemime reviewed Apr 2, 2019
View changes

Show resolved Hide resolved src/item.cpp Outdated
@codemime

codemime reviewed Apr 2, 2019
View changes

Show resolved Hide resolved src/item.cpp Outdated
@ifreund

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Apr 3, 2019 •
edited

Ok, proposed solution for rot that actually better captures what the old code was trying to calculate and I think makes sense.

  • The craft entity essentially mimics the comestible stats of the product
  • On creation of the craft entity transfer the relative rot of the most rotten component to the craft entity
  • Upon completion transfer the rot from the craft entity to the product and byproducts
  • If the craft fails and components are returned, apply the delta relative rot of the craft item since creation

@ifreund ifreund changed the title Initial craft entity implementation [WIP] Initial craft entity implementation Apr 3, 2019

@kevingranade

This comment has been minimized.

Sign in to view
Copy link
Member

commented Apr 3, 2019 •
edited

That makes sense to me, and has a number of great qualities.
Only deal with one item while in progress.
It's generous with rot times in case of success since finished products generally have slower rot than inputs.

Only weird edge case, what if an output is permafood?
@ifreund

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Apr 3, 2019 •
edited

Ok, I implemented basically what I outlined, for the permafood edge case I've decided to maintain the status quo of crafting permafood with old/rotting components being an exploit. Will look at fixing that in a future pr.

This is ready for review again and hopefully merge.
Edit: gonna do some more refactoring of the activity thanks to @codemime

@ifreund ifreund changed the title [WIP] Initial craft entity implementation Initial craft entity implementation Apr 3, 2019

@ifreund ifreund changed the title Initial craft entity implementation [WIP] Initial craft entity implementation Apr 3, 2019

@ifreund

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Apr 3, 2019

Ok, now we should really be good to go.

@ifreund ifreund changed the title [WIP] Initial craft entity implementation Initial craft entity implementation Apr 3, 2019

@kevingranade

kevingranade requested changes Apr 4, 2019
View changes

Show resolved Hide resolved tests/crafting_test.cpp
@ifreund

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Apr 4, 2019 •
edited

Gotta love it when the tests catch all the stupid things I did. Should be good to go now.

@ifreund ifreund force-pushed the ifreund:craft-entity branch from 8a60fa1 to 00e9a6a Apr 5, 2019

This was referenced Apr 5, 2019

Merged
Closed
@codemime

codemime reviewed Apr 5, 2019
View changes

src/activity_handlers.cpp Outdated
}
if( !p->has_item( *craft ) ) {
p->add_msg_if_player( "%s no longer has the target '%s.' Aborting ACT_CRAFT.",
p->disp_name(), craft->tname() );

This comment has been minimized.

Sign in to view
Copy link
@codemime

codemime Apr 5, 2019

Member

This can appear during normal play, so it shouldn't look like a debug message.

This comment has been minimized.

Sign in to view
Copy link
@ifreund

ifreund Apr 6, 2019

Author Contributor

Pretty sure I intended for that to be a debug message but called the wrong function. I guess it could pop up rarely in normal play however and wouldn't cause any problems, so I'll take your advice and fix it.

Thanks for all your time reviewing this pr by the way!

This comment has been minimized.

Sign in to view
Copy link
@codemime

codemime Apr 6, 2019

Member

Thanks in return for your efforts working on it: the list of presumably resolved issues is quite impressive.

@ifreund ifreund changed the title Initial craft entity implementation [RDY] Initial craft entity implementation Apr 7, 2019

ifreund added some commits Mar 23, 2019

@ifreund
Initial craft entity implementation
78e7a2f
@ifreund
Reimplement long crafting
4e8f943
@ifreund
Faliure, xp gain, and rot reimplementations 
As well as:
-Apply suggestions from code review
-Fix crafting_interruption test
-Cleanup
808d285
@ifreund
Reimplement comestible craft entitys properly 
-Handle permafood edge case
4be060c
@ifreund
Apply fixes and cleanup 
Including:
-Activity cleanup from code review
-Make can_eat() return false for crafts
-Make get_comestible return type a const reference like it should have been
9862429
@ifreund
Fix interrupt message
85dbfc7
@ifreund
Inherit weight/volume of components not result
Loading status checks…
fbb59eb

@ifreund ifreund force-pushed the ifreund:craft-entity branch from 1c21032 to fbb59eb Apr 8, 2019

@kevingranade kevingranade merged commit 7225789 into CleverRaven:master Apr 9, 2019
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gorgon-ghprb Build finished.
Details

0.E Release automation moved this from Confirmed to Done Apr 9, 2019

Tailoring Overhaul automation moved this from In progress to Done Apr 9, 2019

@kevingranade

This comment has been minimized.

Sign in to view
Copy link
Member

commented Apr 9, 2019

This pull request has been mentioned on Cataclysm: Dark Days Ahead. There might be relevant details there:

https://discourse.cataclysmdda.org/t/whats-happening-in-your-randomly-generated-apocalypse-part-2/14511/1488

@ifreund ifreund deleted the ifreund:craft-entity branch Apr 9, 2019

@Break-Off Break-Off referenced this pull request Apr 12, 2019

Closed

Craft entity can't be wielded with one hand #29500

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.