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

Radio - Replace inventory PFH with CBA loadout handler #1015

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

Freddo3000
Copy link
Contributor

@Freddo3000 Freddo3000 commented Aug 18, 2020

When merged this pull request will:

image

Tested in vanilla arsenal

Co-authored-by: PabstMirror <[email protected]>
@Freddo3000 Freddo3000 requested a review from PabstMirror August 18, 2020 17:25
@PabstMirror
Copy link
Collaborator

maybe make a note in acre_api_fnc_setItemRadioReplacement that it's use is deprecated
I think manually changing variables that are cba_settings isn't good

@Freddo3000 Freddo3000 requested a review from PabstMirror August 18, 2020 17:55
@jonpas
Copy link
Member

jonpas commented Aug 20, 2020

Does this interfere with Basic Mission Setup module? This particular functionality of it is broken atm (ref. #574), but we should still think about it.

addons/sys_radio/XEH_preInit.sqf Outdated Show resolved Hide resolved
addons/sys_radio/initSettings.sqf Outdated Show resolved Hide resolved
@Sniperhid
Copy link
Member

So one of the complexities of moving away from PFH is ACRE_HOLD_OFF_ITEMRADIO_CHECK. I haven't tested it but I don't think your code currently covers this case.

This exists because when right clicking radios on the ground (also crate/other person's inventory etc) they would typically take the assigned item slot. This is less of an issue now that radios inherit from a misc item so they can't go there except ItemRadio. This remains because it's possible to dislodge ItemRadioAcreFlagged which sits in the assigned item slot (to ensure vanilla radio functionality continues). So what the code currently does is on opening inventory is set ACRE_HOLD_OFF_ITEMRADIO_CHECK = true then false after closing the inventory.

So a test case for this is ensuring ItemRadioAcreFlagged remains in the assigned item slot when right clicking on a ItemRadio from a groundholder/weapons crate etc., and that ItemRadioAcreFlagged doesn't exist elsewhere in the inventory.

@Freddo3000 Freddo3000 requested a review from jonpas August 22, 2020 08:43
Copy link
Member

@jonpas jonpas left a comment

Choose a reason for hiding this comment

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

How does this work in combination with Basic Setup module? I think default radios in the module are kind of broken currently anyways.

addons/sys_radio/initSettings.sqf Outdated Show resolved Hide resolved
addons/api/XEH_PREP.hpp Outdated Show resolved Hide resolved
@Freddo3000 Freddo3000 requested a review from jonpas December 14, 2020 10:03
@jonpas
Copy link
Member

jonpas commented Dec 14, 2020

How does this work in combination with Basic Setup module? I think default radios in the module are kind of broken currently anyways.

This question remains and should be looked at. It kind of partly replaces that part of the module as far as I understand.


@Sniperhid can you look at recent changes regarding replacement of PFH with EH?

@Freddo3000
Copy link
Contributor Author

Just as a heads up I haven't tested the last few commits in game

@@ -8,6 +8,7 @@ if (!hasInterface) exitWith {};

["ace_arsenal_displayClosed", {
EGVAR(sys_core,arsenalOpen) = false;
[] call DFUNC(monitorRadiosHandler);
Copy link
Member

Choose a reason for hiding this comment

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

Leaving a note here: We have to think about API for this, copy-pasting those lines is far from ideal.

@jonpas
Copy link
Member

jonpas commented Oct 9, 2021

Will probably take a look at finishing this for 2.10.0.

@jonpas jonpas modified the milestones: 2.9.1, 2.10.0 Oct 9, 2021
@jonpas jonpas modified the milestones: 2.10.0, 2.11.0 May 19, 2022
@jonpas jonpas modified the milestones: 2.11.0, 2.12.0 Jun 15, 2023
@jonpas
Copy link
Member

jonpas commented Jun 15, 2023

This is almost done, but I don't want to risk it release in v2.11.0, will be part of dev-build after though.

@jonpas
Copy link
Member

jonpas commented Sep 14, 2023

Add setting for setting the replacement radio
Add option for simply removing the radio instead without replacement

Split that off into #1283 so we can merge the important part and leave below for further testing.

Change inventory monitoring PFH to instead use CBA loadout player eventhandler

As noted in a comment above, problem is we are potentially not catching all cases of radio replacement with the handler. If we need a special call to handle it for Arsenal, then who knows how many 3rd party systems would need to do the same - that's not reliable. System needs to catch it all without a new API requiring this. PFH as it is shouldn't be expensive in most cases anyways.

@jonpas jonpas changed the title Tweak item radio replacement Radio - Replace inventory PFH with CBA loadout handler Sep 14, 2023
@jonpas jonpas modified the milestones: 2.12.0, Backlog Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants