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

[DONE] Weapon mod rework #5103

Merged
merged 14 commits into from Dec 27, 2013
Merged

Conversation

desrik
Copy link
Contributor

@desrik desrik commented Dec 16, 2013

This is a rework of the current weapon mod system. The current limit of 4 mods per weapon has been removed in favor of this new system.

Adds three new weapon mod_targets: bow, crossbow, and launcher.

It adds the ability for each weapon to specify valid mod 'locations' and the amount of each.
( JSON example: "valid_mod_locations": [[ "accessories", 2 ], [ "grip", 1 ]] means the weapon has two accessory mod locations and one grip mod location. )

Each weapon mod must specify it's mod location. ( JSON: "location": "accessories" means the mod can be installed on any weapon with one available accessory mod location. )

This also makes the pistol crossbow only accept mods that can be installed on both crossbows and pistols. This is to prevent one from attaching, say, a masterkey to the bottom of it.

Current locations:

accessories
barrel
bore
conversion
grip
magazine
mechanism
muzzle
rail
sights
stock
underbarrel

@Rivet-the-Zombie
Copy link
Member

Rivet-the-Zombie commented Dec 16, 2013

The simplest and most immediately useful implementation would be to remove the giant blocks of crappy code that check for redundant attachments while installing weapon mods. So instead of a giant if (weapon.has(scope || sight || holosight || woofer || tweeter)) we could just use flags like if (weapon.has(optics)) to accomplish the same thing.

@desrik
Copy link
Contributor Author

desrik commented Dec 16, 2013

Good idea :)

@NaturesWitness
Copy link
Contributor

NaturesWitness commented Dec 16, 2013

I like this idea but I think the categories could use some tweaking. A very good system can be found in the 1.13 mod for Jagged Alliance 2.

The setup you have is pretty close to 1.13 already, but I do have a few suggestions:
1 - Rename "auxiliary" to "underbarrel", and rename the enhanced grip to foregrip and move it to the underbarrel slot. Also move the bipod to the underbarrel slot.
2 - Add a "rail" slot, and move the laser sight to it.
3 - Add a "muzzle" slot, and move the silencer there.
4 - Add a "scope" slot separate from "sights" for the pistol and rifle scopes.
5 - Split "mechanism" into "internal" and "external".
"internal" gets: Rapid blowback, auto-fire, and waterproofing
"external" gets: Brass catcher, gyro stabilizer, and spare clip
Everything else seems fine

I think this would be pretty effective at allowing things that should stack, and preventing things that shouldn't. I'm really glad you're picking this up, the old mod system always kind of bugged me as being a little oversimplistic and finicky. This should also make it easier to add new weapon categories, like small and large pistols.

@desrik
Copy link
Contributor Author

desrik commented Dec 17, 2013

  1. Renamed to underbarrel, but instead of foregrip i changed it to forward grip. Bipod now underbarrel.
    2)Now have a rail slot, however my google images search says that 99% of the "laser sights" used currently mount underbarrel. ( One of these -> Pistol: https://bitly.com/1kacNtS || Rifle: http://bit.ly/1c86PYq ) So they are now underbarrel. I might make a seperate laser that mounts on the rail but I'm not sure yet.
    3)Gah! Much better that the term I was using :) ( I was calling it tip. )
  2. Why? The only mod that would matter with is improved iron sights. If you have a scope why do you need sights? Scope would be in the way anyways.
    5)I think it would sound better to have 'mechanisms' and 'accessories'. Spare clip, honestly, should be an item, not a weapon mod. But I think I am going to rename ammo to magazine.

EDIT:: I decided to leave the sights category, as I realized that there are actually two mods that would make use of it, not to mention it would be nice to expand on some of these categories.

@NaturesWitness
Copy link
Contributor

NaturesWitness commented Dec 17, 2013

This looks good, I also think the mechanism and accessory slots are MUCH better than internal and external. I think I''ll wait on the crossbow mods thing until this gets landed, just so things don't get tangled. Actually, if you feel like it it might be better to add the bow and crossbow functionality in this PR instead.

@desrik
Copy link
Contributor Author

desrik commented Dec 17, 2013

I can do that if you want. Currently I'm working on giving weapons a new json field that determines available mod locations and amount of said locations. ( Double barrel shotgun with dual masterkeys? ) That might be of more use to your crossbow mods.

@NaturesWitness
Copy link
Contributor

NaturesWitness commented Dec 17, 2013

That sounds like a good idea, especially being able to set available slots by individual weapon; I had just been wondering how I was going to keep underbarrel mods off the pistol crossbow. I'm not sure if allowing multiple items in the same location would work all that well though, for most of the slots it would make no sense (two enhanced stocks on one weapon?). Feel free to implement the crossbow mods if you want to while you're at it, if not I can just work on it after this gets landed.

@desrik
Copy link
Contributor Author

desrik commented Dec 17, 2013

Well, I'm not saying I'm going to give items multiple mod-slots for things that wouldn't be reasonable, but the ability will be there. I wonder if we would be able to use both masterkeys if you mounted one under each barrel with the existing code... If not that would be a thought for a future PR. I actually just finished adding location slots. Testing now. ( Even though I haven't set anything yet that checks for slots except to show them in the inventory screen. )

EDIT::Just realized I'm gonna have to modify the way it shows installed mods so they are shown by slot.

@NaturesWitness
Copy link
Contributor

NaturesWitness commented Dec 17, 2013

Sweet, I had no idea when I started trying to add mods for crossbows it would turn into a total, and awesome, rebuild of the whole gun mod system. Think you for doing all this, what you've come up with is worlds better than anything I could have coded. By the way, now that I look at the values you've set for the mod slots, I agree with your thinking about how some slots should be able to hold more then one mod. I guess I'm so used to the JA2 system, I couldn't really picture it working, but now that I see it, it makes perfect sense.

temp1.str("");
}
if (iternum == 0) { temp1 << (*i).first << ": " << (*i).second << " "; }
else if (!(iternum % 2)) { temp1 << "\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t" << (*i).first << ": " << (*i).second << " "; }
Copy link
Contributor Author

@desrik desrik Dec 18, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it I can only get spaces by using \t and not just a space? That's ugly code, but it's the only way I could get it to work.

desrik added 2 commits Dec 18, 2013
ew json mod_targets: bow, crossbow, and launcher. Removed 4 mod limit in favor of available mod locations. Removed a bunch of if-else statements in favor of a single mod location check. Fixed some issues with item info display. Prbably a few other things I forgot.
used->tname().c_str());
return;
} else if (guntype->skill_used == Skill::skill("launchers") && !mod->used_on_launcher) {
g->add_msg(_("That %s cannot be attached to a rifle."),
Copy link
Contributor

@NaturesWitness NaturesWitness Dec 19, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this say launcher, not rifle?

@NaturesWitness
Copy link
Contributor

NaturesWitness commented Dec 19, 2013

Other than the line note I just added, this looks like it's ready. I can't wait to have this in-game, and also to see what new gunmods people make for bows, crossbows, and launchers now that they can be modded as well.

One final thing, it still feels weird that having an extended magazine disallows using a spare magazine, that's why I suggested moving the spare mag to the accessory slot. Although it wouldn't surprise me if having both an extended mag and a spare on the same weapon would give Cata fits, so if that's the reason I'm fine with leaving it as it is.

… magazine is now in accessories instead of magazine.
g->add_msg(_("You can not use a spare magazine with your %s."),
if (mod->location == "magazine" &&
gun->clip_size() <= 2) {
g->add_msg(_("You can not extend the ammo capacity of your %s."),
gun->tname().c_str());
return;
}
Copy link
Contributor Author

@desrik desrik Dec 20, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight issue here that all my attempts to overcome have failed. Maybe someone else has an idea.

The problem is that currently there is nothing preventing one from attaching say, a grenade launcher, into the underbarrel location of the pistol crossbow. The player should only be able to attach a select few mods to a pistol crossbow. I have tried a few different if checks to no avail. Any ideas on the proper if check of gun id? I tried gun->id and guntype->id and neither trigger the if...

@NaturesWitness
Copy link
Contributor

NaturesWitness commented Dec 20, 2013

Maybe a custom flag on the pistol crossbow in json?

@desrik
Copy link
Contributor Author

desrik commented Dec 20, 2013

Eh, i'm really trying to steer away from using a new flag for one item... There has to be a way to check against the item name or something. Hell, I might have even already had the right check... Could have been in the wrong spot. All I know is I couldn't get a gun->tname(false) == "pistol crossbow" to trigger an if statement. Hopefully someone with better knowledge of the code will come by and shed some light on this. Until then I'm gonna leave it as is.

@NaturesWitness
Copy link
Contributor

NaturesWitness commented Dec 21, 2013

I'm starting to think the best solution may be a dedicated "laser" slot for weapons, as opposed to using the "underbarrel" slot. I just noticed regular bows also use the "underbarrel" slot as well, which seems a little odd. I think it might be best to just use the "underbarrel" slot for larger things like auxiliary weapons and just put the laser somewhere else. I would give bows the "laser" and "sights" slots, adding extra sights on bows is pretty common, and they should be able to use a laser as well. Even if we get the code to recognize the pistol crossbow by it's id, it's still "in the code" as opposed to in the json files so if, for example someone makes some other small crossbow, they have to edit the source code to keep it from using underbarrel weapons.

EDIT - Actually, moving the laser to the "accessory" slot could work too. Also noticed the holographic sight is listed in the "rail" slot as opposed to the "sights" slot with the iron and red-dot sights.

@desrik
Copy link
Contributor Author

desrik commented Dec 21, 2013

I guess that could work, but that nullifies some of my code... The bow has an underbarrel slot in the json, but in-game an attept to attach anything other than a laser sight to a bow results in a message -> "You can only attach a laser sight to your bow."

As for the holographic sight, I'm looking at this -> http://www.natchezss.com/images/products/OSPMRS.jpg

@NaturesWitness
Copy link
Contributor

NaturesWitness commented Dec 21, 2013

I see what you mean with the holo-sight, rail is probably the best slot for it. For the laser, your idea is probably best, I guess we just have to figure out how to get it working. If it becomes a major sticking point though, we can always use either the accessory slot (or just remove underbarrel from the pistol crossbow), get this landed, and keep working on the laser issue in a new PR. It seems a shame to hold up the whole thing because of something fairly small like this.

@desrik
Copy link
Contributor Author

desrik commented Dec 21, 2013

Well I was looking through some other code last night, and I think I might have found a way but I haven't had a chance to code it and see if it does. Will do that as soon as I get a chance.

EDIT:: Also, I think I understand where you were going with the holo-sights. Were you thinking a set of these? -> http://i.imgur.com/2576RWt.jpg?1
These are called fiber-optic sights, and may be a good idea for a mod. ( Increase accuracy in the dark? )

@NaturesWitness
Copy link
Contributor

NaturesWitness commented Dec 21, 2013

I wasn't really sure what the holo-sights were, I just figured since they had the word "sight" in the name they were similar to the iron and red-dot sights. I'm not really an expert on firearms, so I'll defer to your judgement on where things should go.

@desrik
Copy link
Contributor Author

desrik commented Dec 21, 2013

To be honest when I gave most mods their locations I just used gut instinct and the mod description. If those alone weren't detailed enough to make an assumption, I Googled for an image of the mod or something similar. That's how I determined the location for the holographic sight, the laser sight, etc. ( In the case of the laser sight Google actually proved that both an under-barrel and rail mount were feasible, but the under-barrel style is way more common. )

@kevingranade
Copy link
Member

kevingranade commented Dec 23, 2013

My only issue is I'm not a fan of how much info it adds to every gun definition to enumerate all the valid gunmod locations. If I can't come up with a better solution for that It'll work though.

@desrik
Copy link
Contributor Author

desrik commented Dec 23, 2013

The idea was that different guns might have different hardware configurations that might increase or decrease a specific slot number. The numbers that are there now are just a quick run through of the weapons. I'm sure the community will give some input into which guns should have their available locations changed.

As for the way the locations are defined, I couldn't think of a better way to do it, but perhaps you can. I also tried not to limit specific weapons use of this mod as much as I could, the few being the bows and the pistol crossbow, but those were to keep from attaching crazy things to those weapons.

@NaturesWitness
Copy link
Contributor

NaturesWitness commented Dec 23, 2013

The only way I could think to do this without enumerating the slots in the weapon json would be to go to a sort of template system similar to materials. Every gun would have a "configuration" field, like "small pistol", "carbine", or "bull pup assault". Then a separate json file would have a list of every configuration, and what slots every one had available. On the whole though, I'm not sure if that extra level of complexity is needed.

@desrik
Copy link
Contributor Author

desrik commented Dec 23, 2013

@NaturesWitness That's exactly what I was trying to prevent... Doing that prevents guns with odd configurations having mod locations they actually should have. Like a double barrel shotgun... I see no reason why you shouldn't be able to mount two underbarrel mods. It does have two barrels right?

@NaturesWitness
Copy link
Contributor

NaturesWitness commented Dec 23, 2013

The double barrel shotgun can still get 2 underbarrel slots; you just give it it's own template that says it has them. The idea is you can have as many templates as you want, and if 2 or more guns have the same collection of slots, they can use the same template. Really, I think what you have now is fine, I just though I'd throw that idea out there.

@desrik
Copy link
Contributor Author

desrik commented Dec 23, 2013

So if you can make as many templates as you want, what's the difference between the templates and declaring them in the gun?

@NaturesWitness
Copy link
Contributor

NaturesWitness commented Dec 24, 2013

Honestly, not much. Although if there are a lot of guns with the same layout, it could save a little space. Personally I think just doing it your way is fine, I'm not really sure why I suggested this.

@kevingranade
Copy link
Member

kevingranade commented Dec 25, 2013

Yea, I've been thinking on it a good bit too, and I can't come up with a better solution either.
We could perhaps have a template-like baseline, and per-weapon adjustments to it, but that's pretty complex internally, so if we do that we can do it as a later PR.

@desrik
Copy link
Contributor Author

desrik commented Dec 25, 2013

What about moving all the location assignments to a separate json along with the id of the weapon it matches and when loading the weapon, search the locations json for a set that matches the weapons id. If there isn't a matching set, then the weapon is declared as having no mod locations. Then you could eliminate the assignments from the weapon jsons.

@kevingranade
Copy link
Member

kevingranade commented Dec 26, 2013

Yipes, that's even more complicated.
I think what you have now is fine for the moment, and we can look at
patching in templates for the common ones (pistol, rifle, shotgun, etc) to
cut down on duplication later.

@desrik
Copy link
Contributor Author

desrik commented Dec 27, 2013

Ok that sounds good. Antother idea might be to come up with some some basic templates and integrate them into every weapon type, but allow for weapons to overload the template with its own declarations. Meaning, if a weapon has a location template of 1 underbarrel, 1 rail, 1 sight, 1 grip, etc, allow the json field in use now to override any locations assigned. So the previous weapon could have a pistol weapon template, but the weapon could specifically limit, say the accessory slots, to higher or lower than the template, but any assignments in the template that is not redeclared will be used.

@kevingranade kevingranade merged commit fac067a into CleverRaven:master Dec 27, 2013
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants