Skip to content
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

BatchCraft #8808

Merged
merged 4 commits into from Sep 6, 2014

Conversation

Projects
None yet
6 participants
@Robik81
Copy link
Contributor

commented Sep 3, 2014

Adds feature of batch crafting into crafting menu.

  1. open craft menu and select recipe
    bcraft01

  2. press [b]atch to generate batch screen for selected recipe
    bcraft02

  3. either select one of batch recipes, or return to previous screen by pressing [b]atch again

@i2amroy

This comment has been minimized.

Copy link
Member

commented Sep 3, 2014

Seems like you've got some unused parameters present, but if you fix them (and this works, obviously) then I'm behind this totally. Batch crafting would be awesome.

@Rivet-the-Zombie

This comment has been minimized.

Copy link
Member

commented Sep 3, 2014

Yes! I like the sound of this a lot.

@Robik81

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2014

Working on these usused params, will have to break some template methods to avoid them.

@i2amroy

This comment has been minimized.

Copy link
Member

commented Sep 3, 2014

Couldn't you just (void) them in the function? That works to suppress that particular warning without needing to break the template apart for everything else.

FixParam
Hopefully this will work... didn't like the break template variant
either :)
@KA101

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2014

One last thing: make a note in the UI that pressing Batch again will cancel out of batch selection, please? Explanations of new features make good additions great!

UITweak
cycling between "[b]atch" and "cancel [b]atch"
@Robik81

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2014

Done.

In case of bugs, I will swat them tomorrow. Need some sleeping :)

@KA101

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2014

Schlaf gut. Since this is a great place for new bugs to crop up, gonna merge it separately.

@Zireael07

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2014

YEEAH!

@KA101 KA101 self-assigned this Sep 4, 2014

@@ -265,7 +269,7 @@ bool game::check_eligible_containers_for_crafting(recipe *making)
// we go trough empty containers if we need
if (charges_to_store > 0) {
std::vector<item>::iterator iter;
for(iter = conts.begin(); iter != conts.end(); ++iter) {
for(iter = conts.begin(); iter != conts.end();) {
if (iter->is_container_empty()) {

This comment has been minimized.

Copy link
@kevingranade

kevingranade Sep 5, 2014

Member

Looks like this was broken before, but this doesn't fix it. What you want is something like:

for( iter = conts.begin(); iter != conts.end() ) {
    if( iter->is_container_empty ) {
        // Do stuff
        iter = conts.erase( iter );
    } else {
        iter++;
    }
}

This comment has been minimized.

Copy link
@Robik81

Robik81 Sep 5, 2014

Author Contributor

I will check it out. While I was debugging it, it seemed to work just fine. It did not end in endless loop or something.

@@ -689,6 +716,19 @@ recipe *game::select_crafting_recipe()
} else if (action == "RESET_FILTER") {
filterstring = "";
redraw = true;
} else if (action == "CYCLE_BATCH") {
if (current[line]->reversible) {
popup(_("Batch crafting is not available for reversible items!"));

This comment has been minimized.

Copy link
@kevingranade

kevingranade Sep 5, 2014

Member

huh, why not?

This comment has been minimized.

Copy link
@Robik81

Robik81 Sep 5, 2014

Author Contributor

Mostly because it is paint in the ass to implement and it is fringe case. In function
void game::complete_craft()
calling consume_items(&u, *it, making->batch); returns list of used component, which is later used to populate component list in reversible items. Problem is, that in batch crafting, the consumed list is populated with all component for entire batch, which I cannot add to each result, obviously. And while I was thinking about how to separate it, I got a headache from it.

So, I thought that as long as I disable batch craft for these cases, it can be implemented later - if ever, to be honest. Reversible items are not typical examples of items you need to craft in bulk anyway.

@KA101 KA101 removed their assignment Sep 5, 2014

@KA101

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2014

Didn't have the time to give this the attention it needs, but fortunately Kevin's on the case. Thanks, Kevin.

LoopFix
Fixed possible infinite loop as per Kevin's suggestion
@Robik81

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2014

Depending on your containers, that code could indeed end in infinite loop. Thanks for catching it, Kevin.

Question:
Is disabled batch crafting for reversible items merge blocking?

@kevingranade

This comment has been minimized.

Copy link
Member

commented Sep 5, 2014

No, that's an ok reason to put that off, and the error should result in
clear feature requests if people are bothered by it.

@Zireael07

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2014

So this could get merged in, and reversible items could get added by another PR?

@KA101 KA101 added ready for merge and removed in progress labels Sep 6, 2014

@kevingranade kevingranade merged commit 81d498f into CleverRaven:master Sep 6, 2014

@Robik81 Robik81 deleted the Robik81:BatchCraft branch Sep 6, 2014

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.