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

Rearming only rearms the first magazine to a vehicle #5289

Closed
Tuupertunut opened this issue Jun 20, 2017 · 27 comments · Fixed by #5411
Closed

Rearming only rearms the first magazine to a vehicle #5289

Tuupertunut opened this issue Jun 20, 2017 · 27 comments · Fixed by #5411

Comments

@Tuupertunut
Copy link
Contributor

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:

  • Create an M-ATV (HMG).
  • Create an HEMTT Ammo next to it.
  • Go to the M-ATV as gunner and shoot all its ammo until empty. (Note, you have to expend the ammo by shooting it. Simply setting "ammunition" to 0 in the editor does not trigger the bug.)
  • Rearm the M-ATV from the ammo truck.
  • Observe how the M-ATV has only one magazine in its HMG and how the "Rearm"-interaction is no longer available in the ammo truck.

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

Placed Modules:
None

@Dystopian
Copy link
Contributor

@PabstMirror
Copy link
Contributor

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

@dedmen
Copy link
Contributor

dedmen commented Jun 20, 2017

@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
Copy link
Contributor Author

Tuupertunut commented Jun 21, 2017

@PabstMirror Bug was there back in 3.9.2.

@Tuupertunut
Copy link
Contributor Author

@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
Copy link
Contributor Author

Tuupertunut commented Jun 23, 2017

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
Copy link
Contributor

I think magazinesAllTurrets will work correctly

@Tuupertunut
Copy link
Contributor Author

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

@PabstMirror
Copy link
Contributor

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

@Tuupertunut
Copy link
Contributor Author

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

@dedmen
Copy link
Contributor

dedmen commented Jun 23, 2017

@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
Copy link
Contributor Author

@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
Copy link
Contributor

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

@Tuupertunut
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

There is a PR for a full fledged pylon setup menu: #5238

@Tuupertunut
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@PabstMirror
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Tuupertunut commented Jul 10, 2017

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
Copy link
Contributor Author

Tuupertunut commented Jul 11, 2017

@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
Copy link
Contributor

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
Copy link
Contributor Author

Tuupertunut commented Aug 7, 2017

@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
Copy link
Member

jonpas commented Aug 7, 2017

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
Copy link
Contributor Author

PR made #5411

@PabstMirror PabstMirror removed their assignment Aug 29, 2017
@PabstMirror PabstMirror added this to the 3.11.0 milestone Aug 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants