[RDY] Vehicle mounted washing machine #21650

Night-Pryanik · Aug 17, 2017

Washing machine for now spawns only in deluxe RV. Changed it for one of the seats.
It can hold items up to total volume of 40 liters.
Player needs 24 charges (6 liters) of water (decided not to use clean water for washing purposes) and 5 charges of detergent. The machine is connected to the vehicle water tanks, so water is auto-drained on activation. Detergent is auto-consumed from the player's inventory.
Washing program has a fixed duration of 1,5 hours.
Player should wash only filthy items.
Player can manually stop the activated machine before it's completed its program. In that case filthy items won't be cleaned, water and detergent will be simply lost.
Player puts items in the machine and activates it. This means that he closes the lid of the machine, so he can't get his items back until washing machine is stopped - either automatically or manual.
Game will inform player about remaining duration of the washing process every 15 minutes.
There is issue I found - one can still put items in the activated (=closed) machine. That can be abstracted though - imagine it is the machine with upper (not side) lid that can be opened at will, even during the washing process, and player just adds some more items to the machine.
All numbers and values are a subject for discussion.

ZhilkinSerg · Aug 17, 2017

Gorgon says that something is not good with astyle:

21:09:02 astyle regressions found.
21:09:02 Formatted  src/pickup.cpp
21:09:02 Makefile:950: recipe for target 'astyle-check' failed
21:09:02 make: *** [astyle-check] Error 1

AMurkin · Aug 17, 2017

Also it ironically states 21:09:00 +./mainline-build.zsh:87> make clean

ZhilkinSerg · Aug 17, 2017

ZhilkinSerg reviewed Aug 17, 2017

View changes

sorry

ZhilkinSerg · Aug 17, 2017

ZhilkinSerg reviewed Aug 17, 2017

View changes

ZhilkinSerg · Aug 17, 2017

src/map.cpp

+            cur_veh->parts[part].enabled = false;
+            counter = 0;
+        } else if( calendar::once_every( MINUTES( 15 ) ) ) {
+            add_msg( _("It should take %d minutes to finish washing."), time_left / MINUTES( 1 ) + 1 );


Whitespaces are probably required in _ function?

Coolthulhu · Aug 17, 2017

Statics are unacceptable here.
Consider the case where two vehicles have washing machines:

Turn on first machine
Turn on second machine
First machine increments static counter
Second machine increments same exact static counter
Increments repeat for washing_time / 2
First washing machine finishes washing in washing_time / 2
Second machine finishes washing in washing_time * 3 / 2

This only gets worse as more machines are in the bubble.

The easiest way to fix it would be to have washing machine note in its item (vehicle_part::base) when washing started and use this number.

Night-Pryanik · Aug 17, 2017

The easiest way to fix it would be to have washing machine note in its item (vehicle_part::base) when washing started and use this number.

Umm, need some help on this.

Coolthulhu · Aug 17, 2017

Coolthulhu reviewed Aug 17, 2017

View changes

Coolthulhu · Aug 17, 2017

src/pickup.cpp

@@ -83,6 +83,9 @@ interact_results interact_with_vehicle( vehicle *veh, const tripoint &pos,
    const bool can_be_folded = veh->is_foldable();
    const bool is_convertible = ( veh->tags.count( "convertible" ) > 0 );
    const bool remotely_controlled = g->remoteveh() == veh;
+    const bool has_washmachine = ( veh->part_with_feature( veh_root_part, "WASHING_MACHINE" ) >= 0 );
+    bool working = false;


washing_machine_on or something like that. Ambiguous variable names make code harder to read.

Well, I'm not against renaming, but the name has_washmachine was taken by an analogy of has_kitchen, has_weldrig and so on just above.

I think it's about working flag

Oh, sorry, mislooked.

Coolthulhu · Aug 17, 2017

Coolthulhu reviewed Aug 17, 2017

View changes

Coolthulhu · Aug 17, 2017

src/pickup.cpp

@@ -83,6 +83,9 @@ interact_results interact_with_vehicle( vehicle *veh, const tripoint &pos,
    const bool can_be_folded = veh->is_foldable();
    const bool is_convertible = ( veh->tags.count( "convertible" ) > 0 );
    const bool remotely_controlled = g->remoteveh() == veh;
+    const bool has_washmachine = ( veh->part_with_feature( veh_root_part, "WASHING_MACHINE" ) >= 0 );
+    bool working = false;
+    bool detergent_is_enough = false;


Move this variable into the case.
You can have variable declarations inside a case if you wrap the block in braces, like this:

switch( x ) { case y: { bool is_happening = true; if( is_happening ) add_msg( m_good, "It's happening!" ); } }

Move this variable into the case.

You mean move bool detergent_is_enough = false into the USE_WASHMACHINE case?

Yes.
Nothing else uses it.

Moving bool detergent_is_enough = false into the USE_WASHMACHINE case generates crosses initialization of 'bool detergent_is_enough' error.

As I said: you need an extra pair of braces inside the case.
Look at my example above.

Ok, thanks. Fixed in 3af7934.

Coolthulhu · Aug 17, 2017

Coolthulhu reviewed Aug 17, 2017

View changes

Coolthulhu · Aug 17, 2017

src/pickup.cpp

+                    veh->tags.erase( "manual_stop" );
+                    add_msg( m_neutral, _( "You close the lid of the washing machine, and turn it on.  The washing machine is being filled with soapy water." ) );
+                }
+            }


Would be better if you moved the whole block to interact_with_vehicle in pickup.cpp.
Washing machine should not require you to go to vehicle controls to activate it, but rather should be interacted with directly.

But it is already in interact_with_vehicle in pickup.cpp. And it indeed working by interacting with it directly, not through controls.

Oh, my bad then

Coolthulhu · Aug 17, 2017

Umm, need some help on this.

This isn't exactly easy. Vehicle parts aren't easy to work with.

I think I have an easier idea, though:

In interact_with_vehicle in pickup.cpp, check if there is a washing machine
Check if the washing machine is enabled
If washing machine is not enabled, it means it isn't working (duh). Proceed with all the checks for water and detergent. If everything is OK, demand that the player removes all non-filthy items (if any). If there are none, ask the player if they want to enable it. If player wants to enable it, close and enable the machine, and set birthday of all items (or just the first item) inside machine to current turn
If the machine is enabled, it is working. Player can open it, but the items inside will still be dirty AND the machine will become disabled.
In vehicle::idle, check if the first item in any enabled machine is already washed. Like this: first_item.bday + washing_time > current_turn. If it is, disable the machine and clean all the items in machine.

That way it would work similarly to furniture charcoal kiln.

Night-Pryanik · Aug 18, 2017

I was able to get rid of the static counters.
Activating the machine set the bday of the filthy item to the current turn. Then in process_vehicle_items the game checks for the time_left == washing_time - calendar::turn.get_turn() + bday. If there is no time left, stop the process, and clean the filthy clothes.

Coolthulhu · Aug 18, 2017

There are still some issues:

You can wash - and thus apply bday change - to all items, including those that rely on bday to do calculations (food, corpses). You could wash a stack of squirrel corpses to freshen them.
"Washing machine has finished" x20, because you apply finishing per item. Either check only the first item or check them all, then clean them up in a separate loop, then print one message.
Which machine has finished washing? When batteries die, they say which vehicle lost battery power.

EDIT: Also check gorgon/travis complaints.

Night-Pryanik · Aug 18, 2017

You can wash - and thus apply bday change - to all items, including those that rely on bday to do calculations (food, corpses). You could wash a stack of squirrel corpses to freshen them.

Fixed in abaca20.

"Washing machine has finished" x20, because you apply finishing per item. Either check only the first item or check them all, then clean them up in a separate loop, then print one message.

Fixed in da82944.

Which machine has finished washing? When batteries die, they say which vehicle lost battery power.

Fixed in 1b09a2d.

Night-Pryanik · Aug 18, 2017

I just can't to figure out what are the problems with astyle in pickup.cpp.

Coolthulhu · Aug 18, 2017

It would be easiest if you just downloaded the astyle binary version 2.05.1 and ran it (from command line), like this:

astyle.exe --options=path\to\Cataclysm-DDA\.astylerc -n path\to\Cataclysm-DDA\src\pickup.cpp

Astyle sometimes has really stupid requirements that you won't figure out yourself. For example, it may require very strict line breaks that look ugly, or weird alignment.

Coolthulhu · Aug 18, 2017

Coolthulhu reviewed Aug 18, 2017

View changes

Coolthulhu · Aug 18, 2017

src/map.cpp

@@ -4672,6 +4672,28 @@ static void process_vehicle_items( vehicle *cur_veh, int part )
            apply_in_fridge(n);
        }
    }
+
+    const bool washmachine_here = cur_veh->part_flag( part, VPFLAG_WASHING_MACHINE ) && cur_veh->has_part( "WASHING_MACHINE", true );
+    bool washing_machine_finished = false;


You don't use it outside the if, so it should be inside the if.

You mean outside of if( time_left <= 0 ) or if( finished )?

Coolthulhu · Aug 18, 2017

Coolthulhu reviewed Aug 18, 2017

View changes

Coolthulhu · Aug 18, 2017

src/map.cpp

@@ -4672,6 +4672,28 @@ static void process_vehicle_items( vehicle *cur_veh, int part )
            apply_in_fridge(n);
        }
    }
+
+    const bool washmachine_here = cur_veh->part_flag( part, VPFLAG_WASHING_MACHINE ) && cur_veh->has_part( "WASHING_MACHINE", true );


This check is not correct.
Consider more than one washing machine: in this case this check is true if any of the washing machines is turned on.

The check should be:

if( cur_veh->part_flag( part, VPFLAG_WASHING_MACHINE ) && cur_veh->is_part_on( part ) ) {

Fixed in fb0146f.

codemime · Aug 19, 2017

codemime reviewed Aug 19, 2017

View changes

codemime · Aug 19, 2017

src/pickup.cpp

+            bool detergent_is_enough = g->u.crafting_inventory().has_charges( "detergent", 5 );
+            static const std::string filthy( "FILTHY" );
+            bool filthy_items = std::any_of( veh->get_items( washing_machine_part ).begin(),
+                                             veh->get_items( washing_machine_part ).end(),


Though the function doesn't look expensive, it would be safer to stash the result of veh->get_items( washing_machine_part ) in a temp variable: const auto items = veh->get_items( washing_machine_part ).

Implemented in 6114c40, but had to made it without const, as items are changed below, where we set the bday.

AMurkin · Aug 19, 2017

AMurkin reviewed Aug 19, 2017

View changes

AMurkin · Aug 19, 2017

src/pickup.cpp

+            } else {
+                veh->parts[washing_machine_part].enabled = true;
+                for( auto &n : items ) {
+                    if( filthy_items ) {


This condition is redundant.

Fixed in 1163892.

AMurkin · Aug 19, 2017

AMurkin reviewed Aug 19, 2017

View changes

AMurkin · Aug 19, 2017

src/pickup.cpp

+                add_msg( m_bad, _( "You need 5 charges of detergent for the washing machine." ) );
+            } else if( !filthy_items ) {
+                add_msg( m_bad,
+                         _( "There are only non-filthy items in the washing machine.  There is no need to wash them." ) );


Now that message is incorrect.

Fixed in 1163892.

cainiaowu · Nov 1, 2017

Jenkins, rebuild.

codemime · Dec 22, 2017

codemime reviewed Dec 22, 2017

View changes

codemime · Dec 22, 2017

src/map.cpp

@@ -4677,15 +4677,15 @@ static void process_vehicle_items( vehicle *cur_veh, int part )
    bool washing_machine_finished = false;
    if( washmachine_here ) {
        for( auto &n : cur_veh->get_items( part ) ) {
-            const int washing_time = MINUTES( 90 );
-            const int time_left = washing_time - n.age();
+            const time_duration washing_time = 2_hours;


2_hours isn't equivalent to MINUTES( 90 ). Is that intentional?

I don't know how to convert 90 minutes into a new form.

don't know how to translate 90 minutes in to new for.

90_minutes and most probably 1.5_hours will also work

90_minutes is ok. _hours take int so 1.5_hours is not ok.

Night-Pryanik · Jan 8, 2018

I'm sorry for possibly being annoying, but is there something critical that blocks this from merging?

ZhilkinSerg · Jan 8, 2018

I'm sorry for possibly being annoying, but is there something critical that blocks this from merging?

Possibly lack of time for contributors with merge rights.

kevingranade · Jan 25, 2018

kevingranade approved these changes Jan 25, 2018

View changes

kevingranade · Jan 25, 2018

src/pickup.cpp

@@ -83,9 +83,13 @@ interact_results interact_with_vehicle( vehicle *veh, const tripoint &pos,
    const bool can_be_folded = veh->is_foldable();
    const bool is_convertible = ( veh->tags.count( "convertible" ) > 0 );
    const bool remotely_controlled = g->remoteveh() == veh;
+    const int washing_machine_part = veh->part_with_feature( veh_root_part, "WASHING_MACHINE" );
+    const bool has_washmachine = washing_machine_part >= 0;
+    bool washing_machine_on = veh->parts[washing_machine_part].enabled;


Segfault on this line, you cannot dereference an array with the -1 returned by veh->part_with_feature( veh_root_part, "WASHING_MACHINE" ); if there is no washing machine in the vehicle.
This would work:

bool washing_machine_on = ( washing_machine_part == -1 ) ? false : veh->parts[washing_machine_part].enabled;

I fixed this during the merge, looks like it works flawlessly otherwise 👍

Thanks!
Though I'd like to know at which moment does it segfault?

Added info which machine has finished washing.

Loading status checks…

1b09a2d

Night-Pryanik added some commits Aug 18, 2017

Moved variables into the case

Loading status checks…

3af7934

Added info for which machine is washing now.

Loading status checks…

4ab151e

Night-Pryanik added some commits Aug 18, 2017

Astyle

Loading status checks…

39e4ac7

Don't know how this become erased.

Loading status checks…

3c4b68e

Small refactoring.

Loading status checks…

f612ae5

Night-Pryanik added some commits Oct 5, 2017

Replace setting of bday to current turn with the new set_age function

Loading status checks…

1ee5324

Replace calculating time left with the new age() function

Loading status checks…

8dc835e

Night-Pryanik changed the title ~~Vehicle mounted washing machine~~ [RDY] Vehicle mounted washing machine Oct 18, 2017

Added WASHING_MACHINE flag to json_flags.md

Loading status checks…

fb18f0a

Lowered washing program duration from `2_hours` to `90_minutes`

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

f57d05f

Night-Pryanik referenced this pull request Jan 16, 2018
Closed
Batch cleaning of clothes #22776

kevingranade merged commit 8cc9d3f into CleverRaven:master Jan 25, 2018
3 checks passed

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

coverage/coveralls Coverage increased (+0.8%) to 22.783%
Details

gorgon-ghprb Build finished.
Details

Night-Pryanik deleted the Night-Pryanik:vehicle-mounted-washing-machine branch Jan 25, 2018

Night-Pryanik referenced this pull request Jan 31, 2018
Merged
Added vehicle-mounted washing machine deconstruction recipe #22833

CleverRaven/Cataclysm-DDA

Join GitHub today

[RDY] Vehicle mounted washing machine #21650

Conversation

Night-Pryanik commented Aug 17, 2017 • edited

Night-Pryanik added some commits Aug 17, 2017

This comment has been minimized.

ZhilkinSerg commented Aug 17, 2017

This comment has been minimized.

AMurkin commented Aug 17, 2017

ZhilkinSerg reviewed Aug 17, 2017 View changes

ZhilkinSerg left a comment • edited

ZhilkinSerg reviewed Aug 17, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Coolthulhu commented Aug 17, 2017

This comment has been minimized.

Night-Pryanik commented Aug 17, 2017

Coolthulhu reviewed Aug 17, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Coolthulhu reviewed Aug 17, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Coolthulhu reviewed Aug 17, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Coolthulhu commented Aug 17, 2017 • edited

Night-Pryanik added some commits Aug 18, 2017

This comment has been minimized.

Night-Pryanik commented Aug 18, 2017 • edited

This comment has been minimized.

Coolthulhu commented Aug 18, 2017 • edited

Night-Pryanik added some commits Aug 18, 2017

This comment has been minimized.

Night-Pryanik commented Aug 18, 2017 • edited

This comment has been minimized.

Night-Pryanik commented Aug 18, 2017

Night-Pryanik added some commits Aug 18, 2017

This comment has been minimized.

Coolthulhu commented Aug 18, 2017

Coolthulhu reviewed Aug 18, 2017 View changes

This comment has been minimized.

This comment has been minimized.

Coolthulhu reviewed Aug 18, 2017 View changes

This comment has been minimized.

This comment has been minimized.

Night-Pryanik added some commits Aug 19, 2017

codemime reviewed Aug 19, 2017 View changes

This comment has been minimized.

This comment has been minimized.

Night-Pryanik added some commits Aug 19, 2017

AMurkin reviewed Aug 19, 2017 View changes

This comment has been minimized.

This comment has been minimized.

AMurkin reviewed Aug 19, 2017 View changes

This comment has been minimized.

This comment has been minimized.

Night-Pryanik added some commits Oct 5, 2017

Night-Pryanik changed the title Vehicle mounted washing machine [RDY] Vehicle mounted washing machine Oct 18, 2017

This comment has been minimized.

cainiaowu commented Nov 1, 2017

codemime reviewed Dec 22, 2017 View changes

This comment has been minimized.

This comment has been minimized.

Night-Pryanik Dec 22, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Night-Pryanik commented Jan 8, 2018

This comment has been minimized.

Night-Pryanik commented Aug 17, 2017 •

edited

ZhilkinSerg reviewed Aug 17, 2017

View changes

ZhilkinSerg left a comment •

edited

ZhilkinSerg reviewed Aug 17, 2017

View changes

Coolthulhu reviewed Aug 17, 2017

View changes

Coolthulhu reviewed Aug 17, 2017

View changes

Coolthulhu reviewed Aug 17, 2017

View changes

Coolthulhu commented Aug 17, 2017 •

edited

Night-Pryanik commented Aug 18, 2017 •

edited

Coolthulhu commented Aug 18, 2017 •

edited

Night-Pryanik commented Aug 18, 2017 •

edited

Coolthulhu reviewed Aug 18, 2017

View changes

Coolthulhu reviewed Aug 18, 2017

View changes

codemime reviewed Aug 19, 2017

View changes

AMurkin reviewed Aug 19, 2017

View changes

AMurkin reviewed Aug 19, 2017

View changes

codemime reviewed Dec 22, 2017

View changes

Night-Pryanik Dec 22, 2017 •

edited

kevingranade approved these changes Jan 25, 2018

View changes

kevingranade merged commit `8cc9d3f` into CleverRaven:master Jan 25, 2018
3 checks passed