Make multiple use_methods work #11275

Coolthulhu · Feb 16, 2015

An absolutely huge PR that allows items to have multiple use_methods.
Marking as [WIP] until 0.C is out.

The PR is so huge mostly because I had to update not only the system itself, but also some items.
I moved knives and fire melee weapons (fire katana etc.) into iuse_actors.

iuse::knife is now 4 iuse_actors: salvage, inscribe, cauterize and enzlave.

I changed the iuse_actor loading function, because I needed it to be able to load many functions with just a single json object. Otherwise any object with KNIFE function would have to be changed into one with 4 actors, which would bloat the jsons (KNIFE is the most often used function).

At the moment multiple iuse_functions only work with iuse_actor functions (will debugmsg and fail if there's more than one and at least one isn't iuse_actor). This is because only iuse_actors store their own "names".

Item action menu becomes a fair bit more useful: now you can use it to cauterize wounds directly, for example (before you'd have to pick knife->cauterize).

BevapDin · Feb 16, 2015

BevapDin reviewed Feb 16, 2015

View changes

BevapDin · Feb 16, 2015

src/activity_handlers.cpp

-            iuse::cut_up( p, &salvage_tool, &*it, false );
-            p->assign_activity( ACT_LONGSALVAGE, 0 );
-            return;
+    const auto *use_fun = salvage_tool.type->get_use( "salvage" );


Please remove the *, it will be contained in the auto type anyway. It's redundant n most cases.

BevapDin · Feb 16, 2015

BevapDin reviewed Feb 16, 2015

View changes

BevapDin · Feb 16, 2015

src/item_action.cpp

+
+const item_action &item_action_generator::get_action( const item_action_id &id ) const
+{
+    const auto &iter = item_actions.find( id );


Nitpicking: the reference works and is probably completely valid, but it's confusing, why not make it a regular variable?

Rivet-the-Zombie · Feb 16, 2015

Nice!

BevapDin · Feb 16, 2015

BevapDin reviewed Feb 16, 2015

View changes

BevapDin · Feb 16, 2015

src/iuse_actor.cpp

+    moves_per_part = obj.get_int( "moves_per_part", 25 );
+    if( obj.has_array( "material_whitelist" ) ) {
+        JsonArray jarr = obj.get_array( "material_whitelist" );
+        for( int i = 0; i < (int)jarr.size(); ++i ) {


Better: for( size_t i = 0; i < jarr.size(); ...

Or use the "canonical" way (used nearly everywhere else):

while( jarr.has_more() ) { const auto v = jarr.next_string();

Or use JsonObject::get_string_array:

material_whitelist = obj.get_string_array( "material_whitelist" );

Also: I agree with Rivet on this PR.

BevapDin · Feb 16, 2015

BevapDin reviewed Feb 16, 2015

View changes

BevapDin · Feb 16, 2015

src/iuse_actor.cpp

+        it->make( target_id );
+        it->active = true;
+    } else {
+        p->add_msg_if_player( m_bad, _(failure_message.c_str()) );


Careful here: failure_message defaults to an empty string and the translation system will translate the empty string into some debug information about the translation data, you probably don't want that. Check whether the message is empty before displaying it. You can see the translation of the empty string in #10882.

BevapDin · Feb 16, 2015

BevapDin reviewed Feb 16, 2015

View changes

BevapDin · Feb 16, 2015

src/player.cpp

+
+        bool usable = um.get_actor_ptr()->can_use( this, used, false, pos() );
+        const std::string &aname = item_action_generator::generator().get_action_name( um );
+        umenu.addentry( num_total, usable, MENU_AUTOASSIGN, aname.c_str() );


There is an overloaded version of addentry that takes a proper std::string, so you don't need to supply a const char* - call it without the .c_str().

Giving a const char * invokes the string formating, if aname contains a format specifier like "%s", the function will try to access the parameters behind the format and there are none, that's bad.

Note: nice use of the named constant MENU_AUTOASSIGN instead of the literal value 👍

bkentel · Feb 17, 2015

bkentel reviewed Feb 17, 2015

View changes

bkentel · Feb 17, 2015

src/item_action.cpp

            ctxt.register_action("HELP_KEYBINDINGS");
            ctxt.register_action("QUIT");
-            for( auto id : item_actions ) {
+            for( auto id : am ) {


Why not by (const) reference here?

bkentel · Feb 17, 2015

bkentel reviewed Feb 17, 2015

View changes

bkentel · Feb 17, 2015

src/item_action.cpp

                popup( _("You do not have an item that can perform this action.") );
            }
            return false;
        }
 };

-item_action_map map_actions_to_items( player &p )
+item_action_generator::item_action_generator()


If these these are empty, why not = default?

bkentel · Feb 17, 2015

bkentel reviewed Feb 17, 2015

View changes

bkentel · Feb 17, 2015

src/item_action.cpp

@@ -120,9 +129,39 @@ item_action_map map_actions_to_items( player &p )
    return candidates;
 }

+std::string item_action_generator::get_action_name( const item_action_id &id ) const


Is it possible to return a const ref instead? Here and below.

The last change was from const ref to pure string, because BevapDin said it would be less confusing this way. At least that's how I understood it.

I think he meant const auto &iter = item_actions.find( id );.

bkentel · Feb 17, 2015

bkentel reviewed Feb 17, 2015

View changes

bkentel · Feb 17, 2015

src/iuse_actor.cpp

+        material_whitelist.push_back("kevlar");
+        material_whitelist.push_back("plastic");
+        material_whitelist.push_back("wood");
+        material_whitelist.push_back("wool");


You could reserve and emplace_back. (not your code though, I know)

bkentel · Feb 17, 2015

bkentel reviewed Feb 17, 2015

View changes

bkentel · Feb 17, 2015

src/iuse_actor.h

+        virtual long use( player*, item*, bool, point ) const;
+        virtual iuse_actor *clone() const;
+};
+


Default the empty con/destructors for all of the above?

Coolthulhu · Feb 26, 2015

Un-WiPing this.
I updated and tested it and it seems to work (ie. couldn't break it). Should be ready to get merged once the freeze wears off.

Has a minor issue: when a fire weapon runs out of charges, it won't print a message. This isn't easily fixable, but it still works much better than in master branch, where it prints an activation menu when the fuel runs out (as if the player activated the weapon).

DavidKeaton · Feb 26, 2015

What makes it a hard fix? (Since you're waist deep in that code)

Coolthulhu · Feb 27, 2015

Not exactly hard (it can be worked around), but more like a bit hacky - when the fuel runs out, the item function is called with t equal to false. The t parameter is the difference between item being activated by the player (for example selecting a knife) or item "ticking" on its own (active grenade).

This means it's impossible to tell if the item was activated when at low fuel or if it simply ran out of it. I'll probably make another commit where I implement the old method of doing this, but it will be more of a hack than a fix.

Coolthulhu · Feb 27, 2015

OK, got it to work. Wasn't hard, but it isn't exactly the proper solution.
Still, better than what we've got in master.

Coolthulhu · Mar 2, 2015

To sum up changes (and make it easier to test):

Multiple item functions are presented as a menu
KNIFE broken into multiple functions (aliased as "knife")
Action menu supports multi-use items - "use as a knife" action is now 4 actions: "make zlave", "cut up", "cauterize" and "carve writing"
Fire weapons are now json iuse_actors

KA101 · Mar 2, 2015

Large PR does not go in when we're prepping for release, sorry.

kevingranade · Mar 8, 2015

kevingranade reviewed Mar 8, 2015

View changes

kevingranade · Mar 8, 2015

src/item_action.cpp

-item_action_map map_actions_to_items( player &p )
+item_action_generator::item_action_generator() = default;
+
+item_action_generator::~item_action_generator() = default;


I seem to remember mingw having issues with explicit default destructors, or was that something else?

If I recall correctly, explicit default virtual destructors on subclasses are a problem with gcc 4.7 -- it loses the virtual and then complains about it. Shouldn't be a problem here.

Seems like it did break things :/

It's only the combination of virtual and = default that causes problems on older GCCs AFAIK. I haven't take a careful look at this PR, but what's broken that would indicate the defaulted special member functions?

@kevingranade If you're referencing #11530, that's from some other defaulted destructor -- this one is non-virtual and has nothing to do with the it_{,artifact}_{tool,armor} classes.

Yea my bad, I was following along on my phone and didn't have the details.

kevingranade · Mar 8, 2015

kevingranade reviewed Mar 8, 2015

View changes

kevingranade · Mar 8, 2015

src/iuse.cpp

-static hp_part use_healing_item(player *p, item *it, int normal_power, int head_power,
-                                int torso_power, int bleed,
-                                int bite, int infect, bool force)
+hp_part use_healing_item(player *p, item *it, int normal_power, int head_power,


Why un-static this? Is it being called from outside the file now?

Yep - I use it to cauterize from iuse_actor.cpp. Currently as extern, but it would probably work better if all the invocations were actually iuse_actor type - medical item functions are simply checking for under water and then invoking use_healing_item. Except for first aid kit, which also assigns an activity.

narc0tiq · Mar 10, 2015

narc0tiq reviewed Mar 10, 2015

View changes

narc0tiq · Mar 10, 2015

src/activity_handlers.cpp

-        add_msg(m_bad, _("You no longer have the necessary tools to keep salvaging!"));
-    }
-
+    item &salvage_tool = p->i_at( act->values[2] );


Yo, it looks like you broke longsalvage so hard, we still can't figure it out. For reference, longsalvage is "Cut up all you can". I'm not convinced you ever tested it.

Tested it in parts - it worked at start, later on I focused on fireweapons.
Will try to fix it now.

EDIT: Yep, I actually forgot to test longsalvage - did test only "shortsalvage".
Still, should be easy to fix.

Actually, nevermind, looks like @tivec and I managed to figure it out after all. Do take a look at #11524 and make sure it's still sane, though.

Make multiple use_methods work

Loading status checks…

2fbff48

Cleanup

Loading status checks…

b462ba1

Cleanup 2

Loading status checks…

fe048d0

Cleanup 3

Loading status checks…

3264b29

Coolthulhu force-pushed the cataclysmbnteam:multi-use-method branch Feb 26, 2015

Post-merge fixup

Loading status checks…

1d348da

Coolthulhu force-pushed the cataclysmbnteam:multi-use-method branch to 1d348da Feb 26, 2015

Fix fireweapons

Loading status checks…

02d53df

Coolthulhu changed the title ~~[WIP] Make multiple use_methods work~~ Make multiple use_methods work Feb 26, 2015

Fireweapon out-of-ammo and firestarter

Loading status checks…

d4f9b74

kevingranade self-assigned this Mar 10, 2015

kevingranade merged commit d4f9b74 into CleverRaven:master Mar 10, 2015
1 check passed

1 check passed

default
Details

tivec referenced this pull request Mar 10, 2015
Merged
Fixes long salvage segfault. #11524

kevingranade referenced this pull request Mar 11, 2015
Closed
Master branch fails to compile #11530

Coolthulhu deleted the cataclysmbnteam:multi-use-method branch Apr 10, 2015

keyspace referenced this pull request Dec 29, 2016
Closed
Cell phones don't require batteries #16634

CleverRaven/Cataclysm-DDA

Join GitHub today

Make multiple use_methods work #11275

Conversation

Coolthulhu commented Feb 16, 2015

BevapDin reviewed Feb 16, 2015 View changes

This comment has been minimized.

BevapDin reviewed Feb 16, 2015 View changes

This comment has been minimized.

This comment has been minimized.

Rivet-the-Zombie commented Feb 16, 2015

BevapDin reviewed Feb 16, 2015 View changes

This comment has been minimized.

BevapDin reviewed Feb 16, 2015 View changes

This comment has been minimized.

BevapDin reviewed Feb 16, 2015 View changes

This comment has been minimized.

bkentel reviewed Feb 17, 2015 View changes

This comment has been minimized.

bkentel reviewed Feb 17, 2015 View changes

This comment has been minimized.

bkentel reviewed Feb 17, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bkentel reviewed Feb 17, 2015 View changes

This comment has been minimized.

bkentel reviewed Feb 17, 2015 View changes

This comment has been minimized.

Coolthulhu added some commits Feb 17, 2015

Coolthulhu force-pushed the cataclysmbnteam:multi-use-method branch Feb 26, 2015

Coolthulhu force-pushed the cataclysmbnteam:multi-use-method branch to 1d348da Feb 26, 2015

Coolthulhu changed the title [WIP] Make multiple use_methods work Make multiple use_methods work Feb 26, 2015

This comment has been minimized.

Coolthulhu commented Feb 26, 2015

This comment has been minimized.

DavidKeaton commented Feb 26, 2015

This comment has been minimized.

Coolthulhu commented Feb 27, 2015

This comment has been minimized.

Coolthulhu commented Feb 27, 2015

This comment has been minimized.

Coolthulhu commented Mar 2, 2015

This comment has been minimized.

KA101 commented Mar 2, 2015

kevingranade reviewed Mar 8, 2015 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.

kevingranade reviewed Mar 8, 2015 View changes

This comment has been minimized.

This comment has been minimized.

kevingranade self-assigned this Mar 10, 2015

Hide details View details kevingranade merged commit d4f9b74 into CleverRaven:master Mar 10, 2015 1 check passed

1 check passed

narc0tiq reviewed Mar 10, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

tivec referenced this pull request Mar 10, 2015

kevingranade referenced this pull request Mar 11, 2015

Coolthulhu deleted the cataclysmbnteam:multi-use-method branch Apr 10, 2015

keyspace referenced this pull request Dec 29, 2016

BevapDin reviewed Feb 16, 2015

View changes

BevapDin reviewed Feb 16, 2015

View changes

BevapDin reviewed Feb 16, 2015

View changes

BevapDin reviewed Feb 16, 2015

View changes

BevapDin reviewed Feb 16, 2015

View changes

bkentel reviewed Feb 17, 2015

View changes

bkentel reviewed Feb 17, 2015

View changes

bkentel reviewed Feb 17, 2015

View changes

bkentel reviewed Feb 17, 2015

View changes

bkentel reviewed Feb 17, 2015

View changes

Coolthulhu force-pushed the cataclysmbnteam:multi-use-method branch to `1d348da` Feb 26, 2015

kevingranade reviewed Mar 8, 2015

View changes

kevingranade reviewed Mar 8, 2015

View changes

kevingranade merged commit `d4f9b74` into CleverRaven:master Mar 10, 2015
1 check passed

narc0tiq reviewed Mar 10, 2015

View changes