Unified item mods #18735

mugling · Oct 11, 2016

Provides the groundwork for #17008 by implementing a TOOLMOD type. The design is intentionally very simple - only one mod can be installed at once and the only possible restrictions are per ammo type. We don't want nor need anything as complex as the GUNMOD mess.

Pure JSON definition of tool mods can probably also be exploited by mod authors
Replaces multiple iuse functions with iuse::toolmod_attach
Deprecates DOUBLE_AMMO and ATOMIC_AMMO flags
Gets rid of some special cases in crafting code (removal of mods)
Replace hardcoding with explicit toolmod weight and volume/volume_integral
Includes legacy savegame migration

The purpose of this PR is to cleanup core - not to expand the range of tool mods (although mod authors may wish to do so). The following fields are supported:

acceptable_ammo
ammo_modifier
capacity_multiplier
magazine_adaptor
flags

The radio activation mod could probably also later be converted to use this functionality (separate PR)

mugling · Oct 12, 2016

Ready

BorkBorkGoesTheCode · Oct 12, 2016

Could this be fixed? #18739

mugling · Oct 12, 2016

Could this be fixed? #18739

Unrelated to this PR

Coolthulhu · Oct 12, 2016

Has a conflict with #18514 - looks trivial, just a string change.

mugling · Oct 12, 2016

Has a conflict with #18514 - looks trivial, just a string change.

Yes, I've purposely avoided adding requirement and denial reasons to this PR so that it won't cause a problem when we later resume #18514

Coolthulhu · Oct 12, 2016

Received unrecognized iuse function UPS_BATTERY, using iuse::none instead

And then segfault during migration.

mugling · Oct 12, 2016

Will look into. I don't suppose you have a stack trace?

mugling · Oct 12, 2016

Fixed (099a8cf) by removing the useless entry to UPS_BATTERY. An unmatched use_action shouldn't result in a segfault but that's a separate issue to be tackled in another PR.

Coolthulhu · Oct 12, 2016

Spawn each of the battery mods
Spawn flashlights
Apply atomic mod

Expected:

Flashlights are listed
Applying to one unloads it and then applies the mod

Got:

"No compatible tools"
Unload flashlight
Becomes compatible

mugling · Oct 12, 2016

Known limitation to avoid transmutation of ammo. If it's essential we could add an implicit unload step but I'd prefer to wait for #18514 and then write an implementation complete with denial reasons?

Coolthulhu · Oct 12, 2016

Old implementation allowed it.
It should be easy to just remove the ammo before application of the mod.

mugling · Oct 12, 2016

Fixed

Coolthulhu · Oct 12, 2016

Atomic mod lost a 0 in multiplier - it's 10 times capacity, was 100.

mugling · Oct 12, 2016

Atomic mod lost a 0 in multiplier - it's 10 times capacity, was 100.

Intentional although I'm not especially attached to that if you disagree

Coolthulhu · Oct 12, 2016

Atomic batteries are incredibly expensive compared to regular ones, them having "infinite" capacity makes them less useless.

mugling · Oct 12, 2016

Atomic batteries are incredibly expensive compared to regular ones, them having "infinite" capacity makes them less useless.

Ok, fixed

Coolthulhu · Oct 12, 2016

Coolthulhu reviewed Oct 12, 2016

View changes

Coolthulhu · Oct 12, 2016

src/item.cpp

@@ -3558,6 +3559,11 @@ bool item::is_wheel() const
    return type->wheel.get() != nullptr;
 }

+bool item::is_toolmod() const
+{
+    return !is_gunmod() && type->mod;


That's really hacky now that I think about it.

Coolthulhu · Oct 12, 2016

Coolthulhu reviewed Oct 12, 2016

View changes

Coolthulhu · Oct 12, 2016

src/itype.h

+    std::set<ammotype> acceptable_ammo;
+
+    /** If set modifies parent ammo to this type */
+    ammotype ammo_modifier = NULL_ID;


Shit, I merged it because it didn't crash on my build, but NULL_ID in unspecified context (possibly static) is still unsafe.

I'll push an amendment now

I think it wasn't change though from the original?

EDIT: Yes, it wasn't changed

What are the rules about NULL_ID. We need to get that fixed for 0.D?

NULL_ID must never be used in context that can be static.
Static initialization is not guaranteed to happen in any order other than those specified for single file.

For example, on Cygwin (just an example), the sort order of files seems totally scrambled.
I think it's the link order which determines static initialization order. There are ways to guarantee static initialization order, but they most likely depend on very specific linking which would be a pain to keep.

Would probably be better to just drop the static NULL_ID and wrap it in a getter. We had a PR like this and I think I stalled it, but I can't think of a better way to do it.
We could keep the null id type for that weird overload trick, but it should no longer point to possibly-valid object.

Coolthulhu · Oct 12, 2016

Coolthulhu reviewed Oct 12, 2016

View changes

Coolthulhu · Oct 12, 2016

src/iuse.cpp

-    static const std::vector<std::string> removable_mods = {{
-        "DOUBLE_AMMO", "USE_UPS", "ATOMIC_AMMO"
-    }};
+    if( !p ) {


Redundant, p is never null.

Only because we hacked around that. Theres a code comment somewhere about wanting to drop that precondition

Would be much better to hand it over to a null character anyway.

True. I want to rework iuse to use an item_location. What would a good function signature look like?

Character &user, item &it, bool t, const tripoint &pos

If using item_location, you should first guarantee that the supplied location:

Always points to a valid item (this should never need to be checked in the iuses)

Can be accessed in constant time almost always (ie. unpacking would be done only for the first time)

remove_item is never invoked during processing. Otherwise the active item vectors would get scrambled and the function itself could segfault.

So overall it would be better to keep it without location. If we want location, it would be better to keep an item map and give items some sort of id - it would be more work at start, but would most likely end up less work in total.

remove_item could remove the item from the active item vectors. One of the reasons I want to use item_location is that it makes vehicle mounted tools or interaction with static furniture much easier.

Now that would lead to a giant mess, as active item processing would need to understand being tampered with in the middle of iteration.

Coolthulhu · Oct 12, 2016

Coolthulhu reviewed Oct 12, 2016

View changes

Coolthulhu · Oct 12, 2016

src/iuse.cpp

-        p->add_msg_if_player( m_info, _( "You do not have that item!" ) );
+    // can remove mods from unloaded tools only
+    auto filter = []( const item &e ) {
+        return !e.toolmods().empty() && !e.ammo_remaining() && !e.magazine_current();


Wouldn't be hard to just unload them before removing the mods.

Lots of nasty corner cases (plutonium)

mugling force-pushed the mugling:toolmod branch to 1ac1149 Oct 11, 2016

mugling added some commits Oct 11, 2016

Initial implementation of unified item mods

1ac1149

Add linter rules for TOOLMOD

30243bb

mugling force-pushed the mugling:toolmod branch to 09e9d78 Oct 11, 2016

mugling added some commits Oct 11, 2016

Atomic battery becomes TOOLMOD

b68d295

Convert more toolmods to JSON

e606819

Inherit flags from toolmods

d9c83d1

Handle UPS battery mod

9dea818

Display modification status

Loading status checks…

09e9d78

mugling changed the title ~~Unified item mods [WIP]~~ Unified item mods Oct 11, 2016

mugling referenced this pull request Oct 11, 2016
Merged
[RDY]Batteries as magazines mod #17008
3 of 3 tasks complete

mugling added some commits Oct 11, 2016

Support removal of toolmods

Loading status checks…

d7bc691

Improve display of attached toolmods

Loading status checks…

59797b0

Restore missing flags from atomic mod

c86703c

mugling added some commits Oct 12, 2016

Migrate legacy savegames

Loading status checks…

d837239

Drop documentation for deprecated flags

Loading status checks…

3bb3c7b

Coolthulhu self-assigned this Oct 12, 2016

Add missing item action definition

Loading status checks…

2934fe7

mugling added some commits Oct 12, 2016

Fix segfault

Loading status checks…

099a8cf

Auto-unload tools when adding mods

Loading status checks…

e662287

Coolthulhu merged commit 400ac9b into CleverRaven:master Oct 12, 2016

Bump atomic battery capacity

a7e824c

keyspace referenced this pull request Feb 6, 2017
Closed
Bayonet attached as gunmod to gun does not give increased damage, including for reach attacks (regression) #20214

alanbrady referenced this pull request Jun 24, 2019
Closed
magazine_adaptor is not documented #31816

CleverRaven/Cataclysm-DDA

Join GitHub today

Unified item mods #18735

Conversation

mugling commented Oct 11, 2016 • edited

mugling force-pushed the mugling:toolmod branch to 1ac1149 Oct 11, 2016

mugling added some commits Oct 11, 2016

mugling force-pushed the mugling:toolmod branch to 09e9d78 Oct 11, 2016

mugling added some commits Oct 11, 2016

mugling changed the title Unified item mods [WIP] Unified item mods Oct 11, 2016

mugling referenced this pull request Oct 11, 2016

mugling added some commits Oct 11, 2016

This comment has been minimized.

mugling commented Oct 12, 2016

mugling added some commits Oct 12, 2016

This comment has been minimized.

BorkBorkGoesTheCode commented Oct 12, 2016

This comment has been minimized.

mugling commented Oct 12, 2016

Coolthulhu self-assigned this Oct 12, 2016

This comment has been minimized.

Coolthulhu commented Oct 12, 2016

This comment has been minimized.

mugling commented Oct 12, 2016

This comment has been minimized.

Coolthulhu commented Oct 12, 2016

This comment has been minimized.

mugling commented Oct 12, 2016

This comment has been minimized.

mugling commented Oct 12, 2016

This comment has been minimized.

Coolthulhu commented Oct 12, 2016

This comment has been minimized.

mugling commented Oct 12, 2016

This comment has been minimized.

Coolthulhu commented Oct 12, 2016

This comment has been minimized.

mugling commented Oct 12, 2016

This comment has been minimized.

Coolthulhu commented Oct 12, 2016

This comment has been minimized.

mugling commented Oct 12, 2016

mugling added some commits Oct 12, 2016

This comment has been minimized.

Coolthulhu commented Oct 12, 2016

This comment has been minimized.

mugling commented Oct 12, 2016 • edited

Coolthulhu merged commit 400ac9b into CleverRaven:master Oct 12, 2016

Coolthulhu reviewed Oct 12, 2016 View changes

This comment has been minimized.

Coolthulhu reviewed Oct 12, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mugling Oct 12, 2016 • edited

This comment has been minimized.

This comment has been minimized.

Coolthulhu reviewed Oct 12, 2016 View changes

This comment has been minimized.

This comment has been minimized.

mugling Oct 12, 2016 • edited

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 Oct 12, 2016 View changes

This comment has been minimized.

This comment has been minimized.

keyspace referenced this pull request Feb 6, 2017

alanbrady referenced this pull request Jun 24, 2019

mugling commented Oct 11, 2016 •

edited

mugling force-pushed the mugling:toolmod branch to `1ac1149` Oct 11, 2016

mugling force-pushed the mugling:toolmod branch to `09e9d78` Oct 11, 2016

mugling commented Oct 12, 2016 •

edited

Coolthulhu merged commit `400ac9b` into CleverRaven:master Oct 12, 2016

Coolthulhu reviewed Oct 12, 2016

View changes

Coolthulhu reviewed Oct 12, 2016

View changes

mugling Oct 12, 2016 •

edited

Coolthulhu reviewed Oct 12, 2016

View changes

mugling Oct 12, 2016 •

edited

Coolthulhu reviewed Oct 12, 2016

View changes