Modify Ammo on impact

[FS-21]

• When this warhead detonates, it increases the ammo of affected targets by the set amount; a negative value decreases it.

In rulesmd.ini:

[SOMEWARHEAD]   ; Warhead
Ammo=0          ; integer

Note

This will not raise the ammo above the maximum defined by [TechnoType] -> Ammo=, nor below 0.

[github-actions]

Nightly build for this pull request:

• compiled-dll-b586287f011886c4ff4810aa2c78229a59a35538.zip
  These artifacts will expire in 90 days and will not be available for download after that time.

This comment is automatic and is meant to allow guests to get latest nightly builds for this pull request without registering. It is updated on every successful build.

[Starkku]

Looks okay except for couple of small things.

[NetsuNegi]

Ammo may be a concise and clear tag

[FS-21]

Ammo may be a concise and clear tag

What do you mean, reuse the tag name for the warhead tag? I don't remember if Ammo was used by someone else before in WH

[mevitar]

So does this mean that TransferTypes, and other attempts to make warheads modify ammo, were truly abandoned?
Not because i'm agaisnt this, just want to make sure that someone won't revive those older attempts and redo what this does.

[Metadorius]

TransferTypes have been lost in dev limbo even though they're much better than random one-off non-systematic modifiers.

[FS-21]

Applied feedback and renamed the tag from AmmoModifier to Ammo.

[mevitar]

Seems to be working fine, but i didn't do any tests in multiplayer.

[phoboscn-bot]

To Chinese users:
This pull request has been mentioned on Phobos CN. There might be relevant details there:

致中文用户：
此拉取请求已在 Phobos CN 上被提及。那里可能有相关详细信息：

https://www.phoboscn.top/t/topic/712/6

[DeathFishAtEase]

As an old PR that has not yet been reviewed and approved, it obviously needs to be retested after tidying up, so I changed its test status back to "Needs testing".

[mevitar]

I saw it mentioned above as "working fine", but I'm not sure whether it means the game doesn't crash or the feature works properly. According to my investigation, since the very first commit, this statement has had an issue of being read too late, so it should always be treated as 0 regardless of what value is set in the INI, and actual testing also yielded the expected result (no effect). Moreover, as an old PR that has not yet been reviewed and approved, it obviously needs to be retested after tidying up, so I changed its test status back to "Needs testing".

It means it worked properly at the moment i was testing it. Ammunition was being restored as it should have when warhead hit the targets.

If you tested it with an early build then there is no way it would have worked, because the tag name was different back then.

[mevitar]

Also i remember @FS-21 saying he's planning to rewrite this logic, Is that still the case, or is this going to be merged anyway?

Still, i tested the newest build, works fine. Didn't try multiplayer.

[DeathFishAtEase]

I don't intend to push it forward, especially since it currently has a "Needs discussion" label that I don't quite understand. It's just that someone else wanted to use the functionality of this PR but it didn't work as expected after they merged it themselves, and the documentation description was too brief, so I did a simple maintenance. Since FS-21 has plans to rewrite it, I think we should wait for him to push it forward.

It means it worked properly at the moment i was testing it.

Well, it did work at that time, but its code did indeed have a problem, which only manifested when I updated it. Specifically, the warhead used to be read more than once, which masked the above-mentioned error, and this error was fixed in #2128, so it was exposed when I tested it after updating.
I performed the following tests:

• Based on the last synchronized commit of FS-21 (931571a) (which worked), added the fix from Fix the bug where warhead flags are read twice #2128, and then the feature no longer worked;
• Based on the reimplementation completed by diff comparison above (8481267) (which was invalid), after reverting the fix from Fix the bug where warhead flags are read twice #2128, it became working again.

In short, the reason it originally worked was dependent on a Bug, and now that Bug has been fixed, so it is still necessary to correct the placement of its reading code.

[FS-21]

Sorry, but I don't remember mentioning I wanted to rewrite this small feature. In all case what I wanted is to to update all my PRs but due to my little free time I don't know if it is tomorrow or in a week or in a month or in an hour :-S
Last thing I did it was renaming the name due to NetsuNegi feedback.

[DeathFishAtEase]

It sounds like this PR has reached a completed state. :P
@Coronia What is this Needs Discussion for? I think I don't understand what discussion is already or will be taking place here.

[Coronia]

It's still about the usual 'transfer type' thing. As @Metadorius suggested all these value tweak abilities should be made into a generic pattern that can support value change based on the victim's stat, which is why some similar PRs got rejected before

IMO we don't have to implement a whole system right now, but perhaps it's better to decide tag pattern first so it's extendable and can be re-implemented without changing the protocol once a transfer type really happens

[Metadorius]

I don't know, TransferType is a cursed feature, there were like 4 attempts to do it and they all flopped.

[TaranDahl]

Given the upcoming OpenYR, I think Phobos is nearly at the end of its lifecycle. I don't think there is much benefit in considering extensibility here right now; we might as well just get this merged quickly.