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

Common - Use MagazineReloading event for reload mutex #8432

Merged
merged 27 commits into from
Dec 18, 2024

Conversation

LinkIsGrim
Copy link
Contributor

@LinkIsGrim LinkIsGrim commented Sep 8, 2021

When merged this pull request will:

@LinkIsGrim LinkIsGrim force-pushed the common-reload-mutex-event branch from 0c19724 to 6ca4cf3 Compare September 8, 2021 12:47
@Dystopian
Copy link
Contributor

Maybe better to use weaponState's magazineReloadPhase value instead of isReloading var. Also if we ever need reload event magazineReloadPhase can be used in CBA player EH for it. Tag #8356.

@LinkIsGrim LinkIsGrim changed the title Common - Switch Reload Mutex to Gesture Events Common - Substitute Reload Mutex Sep 8, 2021
@LinkIsGrim LinkIsGrim force-pushed the common-reload-mutex-event branch from 2605cd5 to 5b92eb4 Compare September 8, 2021 13:44
@veteran29
Copy link
Member

This breaks IDI-Systems/acre2#1031 by removing ace_common_isReloading

@LinkIsGrim LinkIsGrim force-pushed the common-reload-mutex-event branch from 2ae2bcf to afd3b27 Compare September 8, 2021 14:41
@jonpas
Copy link
Member

jonpas commented Sep 8, 2021

Please don't force push changes, it makes it incredibly hard to review or update reviews based on latest changes.

@jonpas
Copy link
Member

jonpas commented Sep 12, 2021

This breaks IDI-Systems/acre2#1031 by removing ace_common_isReloading

We can always update ACRE2 as well if this gets removed. I don't think it was considered public API anyway?

@PabstMirror PabstMirror added the target/next-arma Requires something in arma dev branch label Sep 25, 2021
@PabstMirror PabstMirror added this to the Ongoing milestone Sep 25, 2021
@Cyruz143
Copy link
Contributor

Cyruz143 commented Oct 7, 2021

Maybe better suited to CBA since it's used in both ACE and ACRE?

@LinkIsGrim
Copy link
Contributor Author

Commands are now available in current Arma patch.

@LinkIsGrim
Copy link
Contributor Author

I haven't had issues reported on community testing with this, but I understand it's a bit sneakier with any bugs that might show up. Can I get another set of eyes on this?

@PabstMirror
Copy link
Contributor

Not working for me, not sure if something changed in 2.08?
Place "Heavy Gunner" in editor
diag_log text format ["Gesture changed: %2 - %3", _unit, _gesture, (weaponState ACE_Player)];

Gesture changed: gesturereloadmmg02 - ["MMG_02_sand_RCO_LP_F","MMG_02_sand_RCO_LP_F","manual","130Rnd_338_Mag",130,0,0]
Gesture changed: aidlpercmstpsraswrfldnon_g01_player - ["MMG_02_sand_RCO_LP_F","MMG_02_sand_RCO_LP_F","manual","130Rnd_338_Mag",130,0,0]
Gesture changed: gesturereloadpistol - ["hgun_P07_F","hgun_P07_F","Single","16Rnd_9x21_Mag",17,0,0]

Reloading big gun and pistol both fail to trigger the mutex, it seems like weaponState # 6 is 0 at time the gesture event runs

@LinkIsGrim
Copy link
Contributor Author

Back to WIP it is.

@LinkIsGrim LinkIsGrim changed the title Common - Substitute Reload Mutex WIP - Common - Substitute Reload Mutex Mar 9, 2022
@LinkIsGrim LinkIsGrim marked this pull request as draft March 9, 2022 05:38
@LinkIsGrim LinkIsGrim changed the title WIP - Common - Substitute Reload Mutex Common - Substitute Reload Mutex Jul 8, 2023
@LinkIsGrim LinkIsGrim marked this pull request as ready for review July 8, 2023 22:07
@hypoxia125
Copy link
Contributor

discord.com/channels/105462288051380224/108187245529268224/1244681061535973457

Thanks, I hate it.

I'm just here to make more problems lol

@LinkIsGrim
Copy link
Contributor Author

LinkIsGrim commented May 30, 2024

If it works as advertised it'll be a more reliable solution. This PR already needs 2.18 anyway.

I just didn't want to rewrite again.

@johnb432 johnb432 removed the 2.18 label Nov 3, 2024
@johnb432
Copy link
Contributor

johnb432 commented Nov 3, 2024

@LinkIsGrim Can you take a look at the changes I made?

@LinkIsGrim
Copy link
Contributor Author

What happens if weapons are switched mid-reload to an empty weapon (is that possible?)

@johnb432
Copy link
Contributor

johnb432 commented Nov 3, 2024

What happens if weapons are switched mid-reload to an empty weapon (is that possible?)

I removed the weapon during reload and it broke. I'll see if I can find a fix.

@johnb432
Copy link
Contributor

johnb432 commented Nov 3, 2024

Fix found and pushed.

@johnb432 johnb432 marked this pull request as ready for review November 17, 2024 09:08
@johnb432 johnb432 modified the milestones: Ongoing, 3.18.2 Nov 17, 2024
johnb432
johnb432 previously approved these changes Nov 17, 2024
Copy link
Contributor

@johnb432 johnb432 left a comment

Choose a reason for hiding this comment

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

It would be best if someone else also approved this, given the changes I've made.

@PabstMirror PabstMirror self-assigned this Nov 17, 2024
@PabstMirror
Copy link
Contributor

[] spawn {
player reload [];
sleep 0.01;
systemChat str ace_common_isReloading;
"ace_gestures_Forward" call ace_gestures_fnc_playSignal;
};

prints false and magazine disappears
and ace_common_isReloading is locked to true (I think line 511 never completes)
the new way seems to be 1-2 frames behind the old way
but the simplification may still be worth it, it's pretty hard to time a reload and gesture keybind at the same time

also both old and new fail on

player reload [];
"ace_gestures_Forward" call ace_gestures_fnc_playSignal;

happening in same frame

@johnb432
Copy link
Contributor

player reload [];
"ace_gestures_Forward" call ace_gestures_fnc_playSignal;

magazineReloadPhase from weaponState returns >0 when run the code, oof. I'll need to think what should be done (maybe have a variable to check if magazineReloadPhase decreases over time?).

@johnb432
Copy link
Contributor

Managed to find a fix for gestures breaking the reload mutex.

Copy link
Contributor Author

@LinkIsGrim LinkIsGrim left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@johnb432 johnb432 left a comment

Choose a reason for hiding this comment

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

Just added the same check for the reload phase getting stuck when units are switched.

@LinkIsGrim LinkIsGrim changed the title Common - Use gesture events to determine if player unit is reloading Common - Use MagazineReloading event for reload mutex Dec 18, 2024
@LinkIsGrim LinkIsGrim merged commit 4916ec5 into acemod:master Dec 18, 2024
3 checks passed
@LinkIsGrim LinkIsGrim deleted the common-reload-mutex-event branch December 18, 2024 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Release Notes: **IMPROVED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Common - Look at improving reload mutex in 2.06
9 participants