Adds easier repairing of simple ranged weapons #13784

chaosvolt · Oct 19, 2015

This SHOULD hopefully not be a glorious clusterfuck like last time I touched the source.

Before starting this PR, I asked on the "needful things" thread whether it'd make sense to allow repairing damaged primitive ranged weapons without gunsmithing tools. I got the obvious answer that maybe they shouldn't even allow accurizing them at all, but no one answered the more important question of whether it would be sensible to allow repairing, but not reinforcing, simple weapons like bows.

I'm not exactly confident in my ability to code, so last effort I made was...discouraging. Hence attempting to solicit opinions on the idea BEFORE making a PR. At that point I figured either I keep pestering people for feedback, start a thread solely to make that suggestion (and hope THAT actually got feedback), or just PR it and hope it isn't immediately shot down as being a stupid idea.

Adds a flag to a selection of simple ranged weapons. Some bows, bullwhips, slings, some crossbows, etc.
Adds checks in the functions for repair kits and soldering irons to allow repairing, but not reinforcing, of items with the relevant flag.

Note that sewing does not include any checks as-is, so slings, bullwhips, etc ALREADY can be repaired via sewing. In fact, they can be reinforced/accurized by sewing too. My next step will be seeing if I can wrangle the sewing function to exclude guns lacking this flag.

chaosvolt · Oct 19, 2015

Oh fuck me. Now I'm seriously wishing I could actually compile a build without an endless series of errors and other fuckups.

kevingranade · Oct 19, 2015

Just FYI, needful things is a terrible place to ask for feedback, if you want feedback, open a new thread. Needful things is for suggesting simple additions to the game, not eliciting feedback on ideas, and in general if you're actually looking for a response of any kind, open a new thread, it makes it stand out more.

As for the idea itself, put some thought into how the repair is going to work. For a damaged bow for example, what are you going to do to restore it to full functionality once it's been warped or cracked? I have a few ideas how that might work, but none of them are in the game right now. Likewise, how are you going to repair a damaged bullwhip? Both items derive their strength from the item acting as a single object, repairs aren't going to go so well.

chaosvolt · Oct 19, 2015

Ack. x.x

True, I'm not sure if it makes sense, but...how does repairing a similarly-cracked bow using the tools inside a gunsmithing repair kit make any more sense? >.>

EDIT: Additionally, the main alternative would be making bows irreparable, which would likely be an example of a nod to realism that detracts from gameplay rather than compliments it.

chaosvolt · Oct 19, 2015

There's also the fact, well...for most of the items this PR applies to, there's nothing stopping the player from exploiting the recipe being reversible. ._.

vache · Oct 19, 2015

Aren't the materials required to make a bow pretty abundant? I see no problem with forcing the player to make a new bow if so.

chaosvolt · Oct 19, 2015

Aside from the added hassle, true.

That...hmm. Though I should've gone with my initial gut instinct that this would be dismissed as a stupid idea. But to put it bluntly, right now having someone else weighing in on this, even if the reaction is negative, is reassuring.

Rivet-the-Zombie · Oct 19, 2015

I'm okay with this one; we can repair a hazmat suit from shredded tatters to factory fresh with a soldering iron, some cut up water bottles, and sufficient skill, so why not this? Equipment wear and maintenance are a huge thing in DDA, so this fits nicely.

chaosvolt · Oct 19, 2015

Hmm. I'm still not sure, but...

Also of note, as mentioned earlier, sewing kits/needles ignore the distinction between gun and not-gun at present, allowing one to reinforce slings or bullwhips with a sewing needle.

Tailoring kits however, DO check for whether the item in question is a gun in their initial function. Expanding this concept to sewing will require shifting the checks to the relevant sub-functions so that lining/padding/etc of slings is disallowed (wouldn't be a useful ability even if allowed), while still allowing this function to do the "can repair, but not reinforce" check on sew-able ranged items.

kevingranade · Oct 19, 2015

True, I'm not sure if it makes sense, but...how does repairing a similarly-cracked bow using the tools inside a gunsmithing repair kit make any more sense? >.>

This is a great example of why "since the game allows X, it should also allow Y" isn't valid. The fact that you can use a gunsmithing kit to do this at all is an oversight. I rarely say, "what does the game do?" to decide if something is reasonable (an exception is balance), I generally say "does this thing itself make sense?".
I'm leaning toward disallowing repair of most of the items you mentioned, but I'm not adamant about it.

There's also the fact, well...for most of the items this PR applies to, there's nothing stopping the player from exploiting the recipe being reversible. ._.

That wouldn't be exploitation IMO, that's exactly how you'd fix e.g. a bullwhip, by disassembling it (which should lose some material) and then re-assembling it along with some extra material. Now for a bow you should almost always lose the body of the bow, making it pretty useless in general, but that's pretty accurate.

chaosvolt · Oct 19, 2015

Hmm. And I see you elaborating on your objections to this PR by questioning the logic of it, but making no such objection when @Rivet-the-Zombie presents the same "if we can X, why can't we Y" logic.

Just...I've been trying to avoid expressing my recent frustrations with you publicly, but it's getting increasingly difficult. I've been more and more reluctant to do damn near ANYTHING on here as of late.

kevingranade · Oct 20, 2015

Yes, Rivet made the same argument, and is equally wrong IMO. I picked on your statement because it was a clear illustration of what's wrong with the argument, not because you made it.

chaosvolt · Oct 20, 2015

Right. Given recent events, I have very little reason to trust the actual motivation behind that.

Rivet-the-Zombie · Oct 20, 2015

I personally love the way how DDA currently goes with you can fix it if it's damaged as opposed to it's damaged and there's nothing you can do about it for two reasons: firstly, that's how it usually works in real life - if it's damaged, you can probably fix it - even bows and bullwhips. Second of all, and this is a game-y reason, having gear that can't be repaired leads to the too cool to ever actually use scenario where the player forgoes using their equipment because they know that wear and tear will add up, and there's no means of maintaining it when that happens.

@chaosvolt, please ease up on the snark; it only creates friction.

chaosvolt · Oct 20, 2015

Understood. Just...have had my mood soured quite a bit lately. As I've said, I'm a bit surprised by now that someone's actually thinking this is a good idea.

kevingranade · Oct 20, 2015

I did not say, "Your PR is wrong", I said, "your argument is invalid". I'm asking for a better argument (which Rivet provided BTW). I've said the same literally dozens of times to different people, and I know you know that, because I've seen you telling other people about it, so you taking it as a personal attack is absurd.
If you can't separate a criticism of your argument or your contributions from an attack on yourself, you're going to have a hard time, because one of my main responsibilities as project leader is to criticize people's contributions, and I'm not going to stop doing it because of your persecution complex.

As for the PR itself, looks good, Rivet addressed the only concern I had, and the code looks solid.
Minor notes:
The flag name bugs me a little, since they aren't actually guns at all, maybe "PRIMITIVE_RANGED_WEAPONS"?
Could you prevent gunsmithing kits from working on the flagged weapons?

chaosvolt · Oct 20, 2015

...hmm. Right then. Could possibly add a check to the gun repair function. What sort of message would be appropriate as a response if you tried to select a now-invalid ranged weapon to repair?

Also should work on adjusting the sewing functions to be consistent with how repair kits and soldering handle this.

EDIT: And, in any case, I'm sorry about how I reacted to that criticism.

Coolthulhu · Oct 20, 2015

What sort of message would be appropriate as a response if you tried to select a now-invalid ranged weapon to repair?

"This weapon is not a firearm" or even just "this item is not a firearm".

Also should work on adjusting the sewing functions to be consistent with how repair kits and soldering handle this.

If you want to generalize it, don't just copypaste the checks, but make a single function for it. It can be defined only in iuse.cpp (because it would be only used there) or as a method for item.
Something like

bool is_firearm(const item &it)
{ 
    return it.is_gun() && !it.has_flag("NOT_REALLY_A_GUN,_ACTUALLY_A_BOW");
}

Of course with the flag name and gun check replaced with the actual values.

chaosvolt · Oct 20, 2015

"This weapon is not a firearm" or even just "this item is not a firearm".

Probably, though adding a clarification that the item can be repaired by mundane means might or might not be useful.

And that could work, unsure how best to streamline the checks, especially for actions where the status of the item is checked multiple times (like with checking whether the weapon is a firearm, then later on checking whether the item is a bow AND already at 100% condition).

chaosvolt · Oct 20, 2015

There we go. I was unsure whether the pneumatic bolt driver or pneumatic assault rifle are gunlike enough to still have them count as firearms, but they are for now, especially since I didn't give the new flag to BB guns. It would make sense that, whatever the final decision is with airguns, it would be applied consistently.

Zireael07 · Oct 20, 2015

IMHO pneumatic stuff should count as firearms. How complex is the BB gun as far as trigger/barrel are concerned?

chaosvolt · Oct 20, 2015

Sadly we lack proper airguns, but yeah.

Now, assuming Jently doesn't spot any fuckups I missed, the only remaining oddity SHOULD be that you can potentially kevlar-pad wearable simple ranged weapons. The code for enhancing is such a tangled mess that I'm unsure where to put any checks for that, but a kevlar-enhanced bow would be of questionable use.

chaosvolt · Oct 20, 2015

Um. Crap. Either I placed @Coolthulhu's definition code for is_firearm in the wrong location, or I misused it...

Coolthulhu · Oct 20, 2015

You added it as a global function, but are using it as an item method. Either declare it as an item method or use it as a global function that takes const item& as an argument.

chaosvolt · Oct 20, 2015

Hmm. And how do I use it as an item method? I would assume it needs to be defined in the relevant functions instead of elsewhere in the file?

If that's the case, it seems like that would entail more copy-pasting and excess lines of code than the way I had it. ._.

Coolthulhu · Oct 20, 2015

Just copy my definition and use it as is_firearm( gun_item_here ).
Adding it as a method would make sense if this method was to be invoked in more than one place. But since it is used only in iuse.cpp, it's OK for it to be a static bool is_firearm declared on top of the file and only used in iuse.cpp.

vache · Oct 20, 2015

Your definition and function body for is_firearm don't match, so it's saying "I have this function with no definition, and I have this definition with no function."

chaosvolt · Oct 20, 2015

You've got to be shitting me. I literally just copy-pasted according to Coolthulhu's instructions. I fucked up code that was practically handed to me on a silver platter? >_>

Coolthulhu · Oct 20, 2015

You changed your copy of the function at one point.

chaosvolt · Oct 20, 2015

Because it error'd at me, saying to use foo->blah instead of foo.blah. >_>

So taking the code as-is made an error that I've already encountered, and fixing the obvious error made it dive headfirst into crazytown. ._.

chaosvolt · Oct 20, 2015

At this point I'm wondering if defining an is_firearm property is really worth the hassle, as it seems to involve adding MORE code than just checking for the two existing properties in the relevant functions, which was working fine.

Not to mention, well...the code you gave me didn't work as-is, and my attempts to fix it made things worse.

Coolthulhu · Oct 21, 2015

Coolthulhu reviewed Oct 21, 2015

View changes

Coolthulhu · Oct 21, 2015

src/iuse.cpp

@@ -7282,7 +7288,7 @@ int iuse::misc_repair(player *p, item *it, bool, const tripoint& )
        p->add_msg_if_player(m_info, _("You do not have that item!"));
        return 0;
    }
-    if (fix->is_gun()) {
+    if ( fix->is_gun() && !fix->has_flag( "GUN_SIMPLE" ) ) {


Here's one of the reasons why I recommended using a function.

chaosvolt · Oct 21, 2015

Hmm. If I knew how to take the function you gave me and unbork it, I would. ._.

Coolthulhu · Oct 21, 2015

Copy it above the first function in which it would be used, then invoke as:

is_firearm(itm) if itm is not a pointer (ie. has its methods invoked with a dot, like itm.has_flag)
is_firearm(*itm) if itm is a pointer (ie. has its methods invoked with a ->, like itm->has_flag)

chaosvolt · Oct 21, 2015

...I did.

https://github.com/chaosvolt/Cataclysm-DDA/commit/5c968c7cbc9294374d15f3e8b378872f59c39318 https://github.com/chaosvolt/Cataclysm-DDA/commit/2904dc850109bd2c3bd20b43a6d1fec5767bb0b2 https://github.com/chaosvolt/Cataclysm-DDA/commit/0a3cbfe2320b4c6d9d841b76e0110f6461c0d3a6

Coolthulhu · Oct 21, 2015

chaosvolt@5c968c7

You replaced the reference with a pointer, then invoked methods of said pointer. Methods which pointers do not have.

chaosvolt@0a3cbfe

Here you deference the pointers to get references, but you changed the function definition to require a pointer rather than a reference.

chaosvolt@2904dc8

This one is closest to being correct. It only needs to have pointers deferenced. Note that not all itms are pointers, only those that were used with ->.

Pointers are those values which are declared with a * in type. For example, in sew_advanced, you have item *mod, which means that mod is a pointer and has its method invoked with a -> and not a ..
To deference a pointer, you add a *. In this case, is_firearm(*pointer_to_item).
Internally, when you type itm->, it is treated as (*itm).. So, for example, itm->has_flag is the same as (*itm).has_flag.

chaosvolt · Oct 21, 2015

Hmm. That's....ah. So on my last commit, all I had wrong was getting some pointers and non-pointers mixed up?

The error it spat up implying the function was causing problems seems a tad odd though.

chaosvolt · Oct 21, 2015

Now, this time if it fails, I'm not touching it until I know what the hell I'm doing. -_-

EDIT: This. That's why I changed the function from how you originally had it. But I don't know how to fix it, as doing what I did in https://github.com/chaosvolt/Cataclysm-DDA/commit/0a3cbfe2320b4c6d9d841b76e0110f6461c0d3a6 made the subsequent error even crazier.

src/iuse.cpp:2394:14: error: member reference type 'item *' is a pointer; maybe you meant to use '->'?
    return it.is_gun() && !it.has_flag("PRIMITIVE_RANGED_WEAPON");
           ~~^
             ->
src/iuse.cpp:2394:30: error: member reference type 'item *' is a pointer; maybe you meant to use '->'?
    return it.is_gun() && !it.has_flag("PRIMITIVE_RANGED_WEAPON");

chaosvolt · Oct 21, 2015

Oh. Shit, I see now. It seems I changed that from your original instructions. Odd. I remember changing the parts INSIDE the definition, but i don't remebering why I changed that part of it. ._.

chaosvolt · Oct 21, 2015

Now I'm trying to figure out why I yanked the const from that in the first place. o.O

EDIT: It looks like I had the const added correctly in https://github.com/chaosvolt/Cataclysm-DDA/commit/3c8367b5d6f129cc36d3b65a83d30aa9ec4dcfe0 and https://github.com/chaosvolt/Cataclysm-DDA/commit/0c2a6011d82e6c481a32443a66bfec5fb4cac3ca to begin with, but fucked it up in https://github.com/chaosvolt/Cataclysm-DDA/commit/5c968c7cbc9294374d15f3e8b378872f59c39318 for some reason.

EDIT 2: I can't see anything in the second commit's error dialogue to explain why I changed that part in the third, though the sheer quantity of errors makes it hard to sort through. ._.

Rivet-the-Zombie · Oct 21, 2015

There's nothing wrong with making mistakes; it's a good way to learn what does and doesn't work.

chaosvolt · Oct 21, 2015

Right. I'm still unsure what prompted me to edit that part though. It's less frustrating and more perplexing.

chaosvolt · Oct 22, 2015

Hmm. Also, any other mod weapons might warrant the use of this flag? I recall excluding the "wrist dread" from Icecoon's Weapon Pack because it wasn't primitive enough to warrant this flag, but having expanded the flag to non-guns in general, it might warrant the flag now.

Other than that, I could presumably use this for Arcana mod magic ranged weapons, as none of them are gunlike at all. But on the other hand, I could afford to wait and give them this flag next time I do a PR for the mod. I am planning to do such a PR if #13788 is merged, as that gives me a crapload of new options.

chaosvolt · Oct 24, 2015

And thank you for the merge.

BevapDin · Oct 24, 2015

BevapDin reviewed Oct 24, 2015

View changes

BevapDin · Oct 24, 2015

src/iuse.cpp

@@ -3366,6 +3376,9 @@ int iuse::solder_weld( player *p, item *it, bool, const tripoint& )
            }
            fix.damage = 0;
        }
+    } else if (fix.damage == 0 || fix.has_flag("PRIMITIVE_RANGED_WEAPON")) {


This || looks suspicious. It means any item that has 0 damage will match the condition, which means one can not reinforce any item at all. Ever. Is that really intended? Extra peculiar because the other two places that have similar logic use &&, which makes more sense.

BevapDin · Oct 24, 2015

BevapDin reviewed Oct 24, 2015

View changes

BevapDin · Oct 24, 2015

src/iuse.cpp

@@ -3234,7 +3244,7 @@ int iuse::solder_weld( player *p, item *it, bool, const tripoint& )
    }};

    int pos = g->inv_for_filter( _("Repair what?"), [it]( const item &itm ) {
-        return itm.made_of_any( materials ) && !itm.is_ammo() && !itm.is_gun() && &itm != it;
+        return itm.made_of_any( materials ) && !itm.is_ammo() && is_firearm(itm) && &itm != it;


Other places in this PR replace is_gun with is_firearm. In this line, it's replaced as well, but the ! is dropped, too. This inverts the logic. Again: is this intended?

chaosvolt · Oct 24, 2015

Oh shit. This would be essentially typos, actually. Lemme start up another PR... @_@

Various obvious fixes
* Wanna bet I've fucked even more things up? ._.

Loading status checks…

80f2074

Fixes poorly-thought-out fix
* Note to self: count your number or parentheses, you idiot.

Loading status checks…

88b1feb

Restored const
...when did I even DO that? And why? I can't recall any error prompting me to try that, so I must've fucked up and miscopied the instructions.

Loading status checks…

c3d6300

Adds flag to arcana mod ranged weapons
* Only the crossbow is even remotely gunlike anyway.

bd67654

Rivet-the-Zombie self-assigned this Oct 24, 2015

Rivet-the-Zombie added a commit that referenced this pull request Oct 24, 2015

Merge pull request #13784 from chaosvolt/repairing-simple-ranged-weapons
Adds easier repairing of simple ranged weapons

574954c

Rivet-the-Zombie merged commit 574954c into CleverRaven:master Oct 24, 2015

chaosvolt deleted the chaosvolt:repairing-simple-ranged-weapons branch Oct 24, 2015

CleverRaven/Cataclysm-DDA

Join GitHub today

Adds easier repairing of simple ranged weapons #13784

Conversation

chaosvolt commented Oct 19, 2015

This comment has been minimized.

chaosvolt commented Oct 19, 2015

This comment has been minimized.

kevingranade commented Oct 19, 2015

This comment has been minimized.

chaosvolt commented Oct 19, 2015

This comment has been minimized.

chaosvolt commented Oct 19, 2015

This comment has been minimized.

vache commented Oct 19, 2015

This comment has been minimized.

chaosvolt commented Oct 19, 2015

This comment has been minimized.

Rivet-the-Zombie commented Oct 19, 2015

This comment has been minimized.

chaosvolt commented Oct 19, 2015

This comment has been minimized.

kevingranade commented Oct 19, 2015

This comment has been minimized.

chaosvolt commented Oct 19, 2015

This comment has been minimized.

kevingranade commented Oct 20, 2015

This comment has been minimized.

chaosvolt commented Oct 20, 2015

This comment has been minimized.

Rivet-the-Zombie commented Oct 20, 2015

This comment has been minimized.

chaosvolt commented Oct 20, 2015

This comment has been minimized.

kevingranade commented Oct 20, 2015

This comment has been minimized.

chaosvolt commented Oct 20, 2015

This comment has been minimized.

Coolthulhu commented Oct 20, 2015

This comment has been minimized.

chaosvolt commented Oct 20, 2015

Chaosvolt and others added some commits Oct 20, 2015

This comment has been minimized.

chaosvolt commented Oct 20, 2015

This comment has been minimized.

Zireael07 commented Oct 20, 2015

This comment has been minimized.

chaosvolt commented Oct 20, 2015

This comment has been minimized.

chaosvolt commented Oct 20, 2015

This comment has been minimized.

Coolthulhu commented Oct 20, 2015

This comment has been minimized.

chaosvolt commented Oct 20, 2015

This comment has been minimized.

Coolthulhu commented Oct 20, 2015

This comment has been minimized.

vache commented Oct 20, 2015

This comment has been minimized.

chaosvolt commented Oct 20, 2015

This comment has been minimized.

Coolthulhu commented Oct 20, 2015

This comment has been minimized.

chaosvolt commented Oct 20, 2015

This comment has been minimized.

chaosvolt commented Oct 20, 2015

Chaosvolt added some commits Oct 21, 2015

Coolthulhu reviewed Oct 21, 2015 View changes

This comment has been minimized.

Coolthulhu Oct 21, 2015

This comment has been minimized.

chaosvolt commented Oct 21, 2015

This comment has been minimized.

Coolthulhu commented Oct 21, 2015

This comment has been minimized.

chaosvolt commented Oct 21, 2015

This comment has been minimized.

Coolthulhu commented Oct 21, 2015

This comment has been minimized.

chaosvolt commented Oct 21, 2015

Coolthulhu reviewed Oct 21, 2015

View changes

Rivet-the-Zombie merged commit `574954c` into CleverRaven:master Oct 24, 2015

BevapDin reviewed Oct 24, 2015

View changes

BevapDin reviewed Oct 24, 2015

View changes