Drop support for charge rifle

[mugling]

As discussed in #15691 our current implementation of CHARGES guns is poor and has to be handled as a special case in many situations. As a result it's a frequent source of bugs, both previously and currently. Finally it's a major obstacle to refactoring the code to not depend upon directly setting the curammo and charges members.

Yeah, they are probably not worth half the effort they're getting

I'd agree with @Coolthulhu that the maintenance cost is disproportional to the actual benefits considering usage of the one solitary item using this flag is minimal. Therefore this issue is a proposal to drop support for this.

Presently the charger gun is a major obstacle to frequently requested features such as multiple/alternative ammo types and also to generic vehicle turrets (#8082). Therefore it's not a straight case of support vs drop and more a question of priority - do we spend time refactoring it vs dropping it and supporting something else that perhaps has more universal appeal?

[Night-Pryanik]

Never used it, but if there are so many issues and so little sense with it - I'd remove it. Besides, we can always re-add support for it after we get tidy, clean, smart and properly refactored code where we need it.

[vulkans22]

Seconded- I can only speak from personal experience but I used the charge rifle once, found it to be impractical and never used it again. I won't miss it if it's removed, especially if its inclusion is preventing a code refactor.

My other line of reasoning is that there are currently individual guns that cover both its low charge use (The laser pistol and rifle), as well as its high charge use. (The fusion blaster)

[BevapDin]

I'm voting against dropping it.

The quality of an implementation or whether it complicates refactoring should not be relevant at all. By this standard we would have to drop other features as well, and may end up dropping and re-implementing them again and again.

Presently the charger gun is a major obstacle to frequently requested features such as multiple/alternative ammo types and also to generic vehicle turrets (#8082).

Can you explain the the obstacle? I know it requires a additional checks and branches.

Finally: you should ask this question in the forum, where you'll get opinions from a broader public. The forum software may even support a poll.

[mugling]

The quality of an implementation or whether it complicates refactoring should not be relevant at all

Can you expand upon this?

Code quality affects maintenance costs and in this case limits future extension. A little used feature with a well written implementation that causes no other problems is more worthy of this effort to support it than a little used feature that does.

By this standard we would have to drop other features as well

Yes, although I have no other such proposals. I feel its a subjective balance. For example if the charger code resulted in intermittent segfaults (not true) and was rarely used (suspected) there would be an even stronger case for removal. Conversely, a frequently used feature with a horrible implementation still needs supporting (the gunmod refactor I'm still working on being a good example).

Can you explain the the obstacle? I know it requires a additional checks and branches.

The conditionals are somewhat ugly and error-prone but the principle obstacle is to deprecating curammo. The long-term goal is for ammo_set and ammo_unset to replace it and for items with integral magazines to store their ammo discretely in their contents. Similarly I also want to try and make charges a private member of item and reduce the usage to just items counted by charges.

Most aspects of charger guns are a special case. For example sometimes they have no ammo type whilst at others they have CHARGER_GUN_AMMO_ID and their ammo stats depend upon charges which must be updated at specific times (failure to do so being a recurrent error). The charger code contains most of the last remaining usages of set_curammo.

It's not that refactoring is impossible and I don't doubt you'll have some useful suggestions. I'm just not convinced it's worth the effort versus concentrating on other features.

Finally: you should ask this question in the forum, where you'll get opinions from a broader public.

It would be helpful to confirm my suspicion that it's not used much/at all although as I've set out above this is only part of the decision.

[player0981234]

I think removing it is ok for me the shot passes through enemies without exploding this bug has been fixed for all firearms but the charge rifle removing it would be a lazy fix

[BevapDin]

Code quality affects maintenance costs and in this case limits future extension. A little used feature with a well written implementation that causes no other problems is more worthy of this effort to support it than a little used feature that does.

Yes, I agree. But this can be a subjective decision. It just happens that I have a different opinion on the amount of maintenance costs and the amount of being used (and therefor on what wins). That's in no way a hard fact, just an opinion. (You're currently working on that code, so you probably know better about the costs.)

The quality of an implementation or whether it complicates refactoring should not be relevant at all

Can you expand upon this?

The wording might be a bit over the top. If it's an awesome feature, well received by the users and maybe even requested, the implementation (as long as it works) is not as important. We had a few bugs with the explosion code and its implementation was (maybe still is) not optimal, but does that mean we should remove the feature? Having a feature, even with ugly (but working) code, can be more important than not having it at all.

Can you explain the the obstacle? I know it requires a additional checks and branches.

For example sometimes they have no ammo type whilst at others they have CHARGER_GUN_AMMO_ID and their ammo stats depend upon charges which must be updated at specific times (failure to do so being a recurrent error). The charger code contains most of the last remaining usages of set_curammo .

I understand. Just a though: you could have a check in the ammodata and update it there before returning it. This ensures it's always up to date.

[SeanMirrsen]

Maybe just refactor the gun itself to use the new system, rather than drop it? The charge rifle is functionally just a gun with variable effects, a more general system that would allow such behavior could be beneficial. (as far as other potential uses, think Unreal Tournament's rocket launcher, or even something low-tech and hand-cranked, like a battery-less tazer)

I'm not directly against dropping the weapon (hell, I'm not using any energy weapons in my playthroughs), but I'm against dropping potentially interesting functionality because it's inconvenient to continue supporting it.

As far as ideas to remake it, maybe a system wherein a weapon has predefined "internal" ammo of several types, which is fired in place of whatever is consumed by the weapon depending on what setting the weapon is set to? As a start. Some other system to draw ammo over time and advance the weapon's internal ammo setting automatically depending on how much ammo (electricity in this case) has been drawn before firing, could happen later and fully restore original functions without impacting the new ammo system.

[Coolthulhu]

If charge rifle was an entire system for generic, variable ammo types based on magazine level, I'd be in favor of keeping it. And building an entire system around it just to justify its existence would be a sin against principles of design (not the first one of its kind, though).

As it stands, it is a single hardcoded item that uses a hardcoded set of functions to fulfill a simple yet not always convenient function. An exception that needs to be special cased rather than acting like everything else.

Couldn't it be moved to an iuse function or something like that?

[mugling]

If charge rifle was an entire system for generic, variable ammo types based on magazine level, I'd be in favor of keeping it

This would make for an interesting feature but is also a considerable quantity of work and I'd prefer to concentrate on fixing the existing magazine functionality in preparation for releasing 0.D

Couldn't it be moved to an iuse function or something like that?

Possibly. I'd probably go with the generic implementation you proposed above if so. Any such re-implementation is going to look nothing like the current one so would likely de-facto involve deprecating the existing.

And building an entire system around it just to justify its existence would be a sin against principles of design

Yes. I'm not going to refactor or extend something that has minimal use simply to justify it's existance.

Just a though: you could have a check in the ammodata and update it there before returning it.

That's a good suggestion. Having item::ammo_data() accessing item::ammo_remaining() doesn't break encapsulation and could at least keep more of the implementation internal to the item class.

[kevingranade]

I don't consider the charge rifle to be of any particular value, it's a very video-game like weapon shoehorned into a game that mostly tries to make the weapons feel real, even if they are fantastical.