acemod / ACE3

Open-source realism mod for Arma 3
https://ace3.acemod.org
Other
1.01k stars 736 forks source link

Rearming only rearms the first magazine to a vehicle #5289

Closed Tuupertunut closed 7 years ago

Tuupertunut commented 7 years ago

Arma 3 Version: 1.70.141838 (stable) CBA Version: 3.3.1 (stable) ACE3 Version: 3.10.0 (stable), also happened in 3.9.2 (stable)

Mods:

- CBA_A3
- ace

Description: Rearming only rearms the first magazine to a vehicle with multiple magazines. For example, the M-ATV (HMG) has 2 HMG magazines. If you shoot them empty, then rearm the vehicle, only one magazine is rearmed. There is no way to rearm it more after that.

Steps to reproduce:

Where did the issue occur? Everywhere, both multi- and singleplayer.

Placed Modules: None

Dystopian commented 7 years ago

arma bug, see https://ace3mod.com/wiki/development/arma-3-issues.html, T79689

PabstMirror commented 7 years ago

Do you know if this has changed with the recent patch (ie. it worked with 3.9.2)?

dedmen commented 7 years ago

@PabstMirror ACE Version states ACE3 Version: 3.10.0 (stable), also happened in 3.9.2 (stable) So I'd read that as "Bug was already present in 3.9.2." But apparently https://feedback.bistudio.com/T79689#1561457 Things changed?

Tuupertunut commented 7 years ago

@PabstMirror Bug was there back in 3.9.2.

Tuupertunut commented 7 years ago

@dedmen The comment you linked seems to talk about a different problem the user "HorribleGoat" had with multiple turrets. He is just saying he found a solution to his problem, but the main bug still exists.

I did some research and the way I see the Arma bug is following:

magazineTurretAmmo is supposed to return the ammo count of a specific magazine. However, it only takes vehicle, magazineClass and turretPath as parameters. If there are multiple magazines in the same vehicle in the same turret with the same magazine class, there are multiple options where it could get the ammunition count. So it is undefined which magazine instance the actual ammo count will be read from.

The same issue is with setMagazineTurretAmmo, which doesn't specify the magazine instance the ammo will be set to.

However, I think there is a workaround. Instead of setting the ammo count to magazines, one could just remove magazine with removeMagazineTurret and then add a new one with addMagazineTurret.

Tuupertunut commented 7 years ago

So the workaround would work perfectly if Rearm amount is Entire vehicle and Ammunition supply is Unlimited. With any other configuration you would always either not rearm completely or lose some ammunition.

Considering that Entire vehicle and Unlimited is the default configuration, would it be worth it to at least fix reaming just for that configuration?

Edit: Whoa! Wait a minute. magazinesAllTurrets gives full detail of ammo level in every magazine in a vehicle! So with magazinesAllTurrets and addMagazineTurret, which allows you to specify the ammo level of the new magazine, I think we can perfectly simulate the working of magazineTurretAmmo and setMagazineTurretAmmo. I'll do some more research to what I can do.

PabstMirror commented 7 years ago

I think magazinesAllTurrets will work correctly

Tuupertunut commented 7 years ago

@PabstMirror Are you fixing this already or will I do it?

PabstMirror commented 7 years ago

I haven't started anything yet, feel free to make a PR

Tuupertunut commented 7 years ago

What does the command "TRACE_7()" do? I can't find any documentation. It seems like some sort of debug logging.

dedmen commented 7 years ago

@Tuupertunut https://github.com/CBATeam/CBA_A3/blob/master/addons/main/script_macros_common.hpp#L409 Yes. It basically writes the values of the Variables to RPT if debug mode is enabled

Tuupertunut commented 7 years ago

@PabstMirror When you added pylon support, did you mean to allow players to change what missiles are in the pylons or just rearm the existing ones?

PabstMirror commented 7 years ago

It was designed to allow changing missile type if supply tuck had the appropriate ammo supply.

Tuupertunut commented 7 years ago

So it will only work when Rearm amount is not Entire vehicle. If it is, it doesn't let you choose the missiles as it just rearms the entire vehicle at once.

Tuupertunut commented 7 years ago

Should changing pylon missiles rather be moved to a separate menu besides "Rearm"? I would see three arguments for that:

  1. You could do it even with Entire vehicle setting.
  2. You could replace missiles in already filled pylons.
  3. You could choose which pylon to attach them to.

Then the "Rearm" menu would be left only for rearming what is already there.

What do you think?

PabstMirror commented 7 years ago

There is a PR for a full fledged pylon setup menu: https://github.com/acemod/ACE3/pull/5238

Tuupertunut commented 7 years ago

So if that PR gets accepted, I can probably remove the pylon reconfiguring feature from rearm.

It seems that @654wak654 is doing some kind of "better integration with rearm component". I will ask him what is it and how it fits to the rearm fixing I'm doing.

Tuupertunut commented 7 years ago

Another question: When Rearm amount is Entire magazine, how should filling partial magazines work?

Lets say you shoot one round out of a magazine and then try to rearm it. When you pick the ammo crate (magazine) from the truck, should that crate only contain one round? If not, then should the entire ammo crate be eaten when you rearm it into the vehicle?

Tuupertunut commented 7 years ago

@PabstMirror Do you or someone else have time to explain this, as it is currently blocking my work?

PabstMirror commented 7 years ago

I've noticed that as well, not really sure how to handle it. I guess for now we can concentrate on fnc_rearmEntireVehicleSuccessLocal.sqf to fix the problem with setMagazineTurretAmmo.

Tuupertunut commented 7 years ago

fnc_rearmEntireVehicleSuccessLocal is working now. It was the easy part. I made replacement functions for both magazineTurretAmmo and setMagazineTurretAmmo. The problem is, fnc_rearmSuccessLocal (which is responsible for rearming in Entire magazine mode), as well as many other functions, use magazineTurretAmmo too.

At this point, I have already fixed everything else but fnc_rearmSuccessLocal, because I don't really know how it is intended to work.

As a side note, I have noticed some weird behavior in fnc_removeMagazineFromSupply, so I'll probably have to fix it as well.

Tuupertunut commented 7 years ago

You can see what I've done up to this point here: https://github.com/Tuupertunut/ACE3

You can discuss/challenge me about any change I've made. :)

Tuupertunut commented 7 years ago

@PabstMirror So this is still an issue. Can you get someone else to explain this:

Lets say you shoot one round out of a magazine and then try to rearm it. When you pick the ammo crate (magazine) from the truck, should that crate only contain one round? If not, then should the entire ammo crate be eaten when you rearm it into the vehicle?

Of course I could just leave Entire magazine rearming untouched, but then this bug wouldn't really be fixed.

PabstMirror commented 7 years ago

Ideally if a tank has a 80/100 magazine, we should only take out 20 bullets from the ammo supply truck. With the limited supply feature this will be a little more tricky. Also account for what happens if player takes that 20 round supply to a different vehicle that needs more ammo.

Like I said I think we could solve this in 2 steps, fixing just rearmEntireVehicleSuccessLocal to actually refill the magazinese and then fixing the limited supply realism part.

Tuupertunut commented 7 years ago

@PabstMirror Back from a long break.

So the current situation is this:

* Rearm amount: Entire vehicle Entire magazine Amount based on caliber
Unlimited ammo supply Fully fixed Vehicle can't be fully rearmed in some cases. Still uses setMagazineTurretAmmo. Vehicle can't be fully rearmed in some cases. Still uses setMagazineTurretAmmo.
Limited ammo supply based on caliber Takes wrong amount from the ammo truck. Takes wrong amount from the ammo truck. Vehicle can't be fully rearmed in some cases. Still uses setMagazineTurretAmmo. Takes wrong amount from the ammo truck. Vehicle can't be fully rearmed in some cases. Still uses setMagazineTurretAmmo.
Only specific magazines Not tested. Not tested. Not tested.

What should I do before making a pull request? Should I fix something more?

jonpas commented 7 years ago

Best if you open a pull request, you can still commit further to the branch once it's open, and that way we can track your progress and comment directly on it. :)

Tuupertunut commented 7 years ago

PR made #5411