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

Arsenal - Make ACE_asItem and ACE_isUnique apply to CfgWeapons #10366

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

Conversation

DartRuffian
Copy link
Contributor

@DartRuffian DartRuffian commented Oct 6, 2024

When merged this pull request will:

  • Title. Use-case is for updating items from other mods to not act as mine detectors and still show in the ACE Arsenal.
    • Updating their simulation / type alone makes them work correctly, but they don't show as misc items since they do not inherit from CBA_MiscItem.

IMPORTANT

  • If the contribution affects the documentation, please include your changes in this pull request so the documentation will appear on the website.
  • Development Guidelines are read, understood and applied.
  • Title of this PR uses our standard template Component - Add|Fix|Improve|Change|Make|Remove {changes}.

@DartRuffian
Copy link
Contributor Author

DartRuffian commented Oct 6, 2024

Seems like some part of this breaks painkillers, as they no longer appear in the arsenal in either the medical tab or misc items tab. I'll take another look in the morning

@DartRuffian DartRuffian changed the title Arsenal - Make ACE_asItem and ACE_isUnique apply to items Arsenal - Make ACE_asItem and ACE_isUnique apply to CfgWeapons Oct 6, 2024
@DartRuffian
Copy link
Contributor Author

Realized I should denote it here, but Grim might have had a different idea, and that I should remind him when 3.18.1 is being looked at

@DartRuffian
Copy link
Contributor Author

Validate reports missing ; on Line 163, but it runs fine

161 {
162     _magazineMiscItems set [configName _x, nil];
163 } forEach (toString {_x call FUNC(isMiscItem)} configClasses _cfgMagazines);

@LinkIsGrim
Copy link
Contributor

LinkIsGrim commented Oct 14, 2024

It's because of the code block. It's fine and the ; will actually make it not work

@LinkIsGrim LinkIsGrim added this to the 3.18.2 milestone Oct 14, 2024
@LinkIsGrim LinkIsGrim added the kind/change Release Notes: **CHANGED:** label Oct 14, 2024
@PabstMirror
Copy link
Contributor

it's Run python3 tools/sqf_validator.py
not hemtt

@LinkIsGrim
Copy link
Contributor

Reading is hard

@LinkIsGrim LinkIsGrim self-assigned this Oct 14, 2024
Copy link
Contributor

@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.

Spare Barrels and Painkillers aren't being shown under Misc Items, so this breaks magazines.

Though, ace_arsenal_fnc_isMiscItem returns true for both.

@DartRuffian
Copy link
Contributor Author

DartRuffian commented Oct 15, 2024

Just FYI, this means magazines will be slower.

Thought about this, maybe optional param for config for the item? I.e.

["someMag", configFile >> "CfgMagazines"] call ace_arsenal_fnc_isMiscItem;

Saves the call to getItemConfig. It also would get around potentially getting the wrong config path back if there's same class names in different root classes

@LinkIsGrim
Copy link
Contributor

Just FYI, this means magazines will be slower.

Thought about this, maybe optional param for config for the item? I.e.

["someMag", configFile >> "CfgMagazines"] call ace_arsenal_fnc_isMiscItem;

Saves the call to getItemConfig. It also would get around potentially getting the wrong config path back if there's same class names in different root classes

Optimize for string and cache it, we make enough lookups for it to be worth it anyway.
I have that ready as soon as I can fix magazine misc items

@DartRuffian
Copy link
Contributor Author

I might mess with it later tonight, no guarantee though

Not fully fixed, as painkillers still show in the misc item tab instead of medical.
@LinkIsGrim
Copy link
Contributor

LinkIsGrim commented Oct 15, 2024

https://github.com/acemod/ACE3/tree/pr/10366/cache moved everything to do with misc items to that function
Things to test:

  • Mine detector still shows up
  • FAKs, Medkits, and toolkits still show up and get sorted properly (need to test with and without medical)
  • Painkillers & spare barrels still show up and get sorted properly
  • Intel Items still show up when picked up but aren't available in a full arsenal

Besides intel items, all need to be tested in an arsenal where they should be available (VR mission is fine) and one where they shouldn't

@PabstMirror PabstMirror changed the base branch from master to release October 15, 2024 20:16
@PabstMirror PabstMirror changed the base branch from release to master October 15, 2024 20:16
LinkIsGrim
LinkIsGrim previously approved these changes Oct 15, 2024
Copy link
Contributor

@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.

Broke it, fixed it, broke it again, fixed it again. Someone else test before merging.

@DartRuffian
Copy link
Contributor Author

I'll test tonight. I'll test the same things you had listed previously as well as an item that originally inherited from ItemCore and then had its type/simulation changed

@DartRuffian
Copy link
Contributor Author

DartRuffian commented Oct 16, 2024

  • Mine detector still shows up
  • FAKs, Medkits, and toolkits still show up and get sorted properly (need to test with and without medical)
    • With ACE Medical
    • Without ACE Medical
  • Painkillers & spare barrels still show up and get sorted properly
  • Intel Items still show up when picked up but aren't available in a full arsenal
    • Not familiar with the intel items, but acex_intelitems_notepad does appear in Tools
  • ItemCore child class with modified type/simulation

@DartRuffian
Copy link
Contributor Author

Test Mission with two arsenals. One full and one limited (there are placed comments with details)
Misc%20Item%20Test.VR.zip

@LinkIsGrim
Copy link
Contributor

Not familiar with the intel items, but acex_intelitems_notepad does appear in Tools

Open Zeus, go to props/empty side units, search for intel > place a document/photo

@LinkIsGrim
Copy link
Contributor

More tests:

  • Check if uniforms/vests/backpacks inside inventory still show up
  • Check if weapons & attachments inside inventory still show up
  • Check if belt items (GPS, Radio, Map, Watch) inside inventory still show up
  • Check if goggles & NVGs inside inventory still show up

These could be in a test script if we cared enough (at least we can check if they're sorted into the right buckets, interface would still need manual checking) I guess @johnb432 ?

@DartRuffian
Copy link
Contributor Author

DartRuffian commented Oct 16, 2024

Open Zeus, go to props/empty side units, search for intel > place a document/photo

Done, I have a acex_intelitems_document in my inventory and it says I have 0 in the arsenal. I assume that's the intended outcome for intel items?
107410_20241016091856_1

@DartRuffian
Copy link
Contributor Author

  • Check if uniforms/vests/backpacks inside inventory still show up
  • Check if weapons & attachments inside inventory still show up
  • Check if belt items (GPS, Radio, Map, Watch) inside inventory still show up
  • Check if goggles & NVGs inside inventory still show up

Never even knew ACE handled these, TIL.
Only issue I found was that the Chemical Detectors are showing under Bipods.

@LinkIsGrim
Copy link
Contributor

LinkIsGrim commented Oct 16, 2024

Never even knew ACE handled these, TIL. Only issue I found was that the Chemical Detectors are showing under Bipods.

They fall into the category of items this PR was made for, which is non-CBA, non-special, CfgWeapons misc items. Adding ACE_asItem to them should sort them correctly.

@LinkIsGrim LinkIsGrim added kind/enhancement Release Notes: **IMPROVED:** and removed kind/change Release Notes: **CHANGED:** labels Oct 16, 2024
@DartRuffian
Copy link
Contributor Author

DartRuffian commented Oct 17, 2024

I'll add it to those classes then. Should they be modified in arsenal itself or another addon?

I assume arsenal, but figured I'd check beforehand.

@DartRuffian
Copy link
Contributor Author

@LinkIsGrim

@LinkIsGrim
Copy link
Contributor

Arsenal

@LinkIsGrim
Copy link
Contributor

inb4 "the chemical detector you guys added doesn't do anything"

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.

3 participants