[RDY] Initial craft entity implementation #28937
Conversation
ifreund
added
Game: Mechanics Change
|
Related suggestion to lessen spoilage of ingredients (especially for large batches): #28930 |
|
Like I said, I'm not aiming to change mechanics too much in this pr. General discussion of the crafting overhaul belongs in #29210 |
|
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
force-pushed
the
craft-entity
branch
2 times, most recently
from
c5336f6 to
e5d845f
Compare
|
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 |
|
I don't see spoilage being handled anywhere in you code. There's a possible exploit related to it:
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. |
|
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. |
|
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. |
|
|
||
| for( const item &it : components ) { | ||
| if( it.has_flag( "HIDDEN_POISON" ) ) { | ||
| set_flag( "HIDDEN_POISON" ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Ok, proposed solution for rot that actually better captures what the old code was trying to calculate and I think makes sense.
|
ifreund
changed the title
|
That makes sense to me, and has a number of great qualities. Only weird edge case, what if an output is permafood? |
|
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.
|
ifreund
changed the title
ifreund
changed the title
|
Ok, now we should really be good to go. |
ifreund
changed the title
|
Gotta love it when the tests catch all the stupid things I did. Should be good to go now. |
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() ); |
There was a problem hiding this comment.
This can appear during normal play, so it shouldn't look like a debug message.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Thanks in return for your efforts working on it: the list of presumably resolved issues is quite impressive.
ifreund
changed the title
As well as: -Apply suggestions from code review -Fix crafting_interruption test -Cleanup
-Handle permafood edge case
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
|
This pull request has been mentioned on Cataclysm: Dark Days Ahead. There might be relevant details there: |
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
craftstart_craft()to generate the itemitem::type_name()(probably put "in progress craft" in the json and move the substitution string to the c++ 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.