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

Medical - Add arsenal category #9220

Merged
merged 8 commits into from
Jul 7, 2023

Conversation

LinkIsGrim
Copy link
Contributor

@LinkIsGrim LinkIsGrim commented Jun 19, 2023

When merged this pull request will:

  • Add "ACE Medical" category to ACE Arsenal through API function

Contains all items in ACE_Medical_Treatment_Actions, compatible with third party mods that add medical items (OPTRE, KAM (though not all items from KAM are added apparently? This should be fixed on their end with API)). Should clean up Misc Items a bit.

Uses Medical GUI icon if that's available. Vanilla medical icon is ugly.

See also #9221.

image

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

LinkIsGrim and others added 2 commits June 19, 2023 03:27
Co-authored-by: PabstMirror <pabstmirror@gmail.com>
@LinkIsGrim LinkIsGrim changed the title Medical - Add category to Arsenal Medical - Add arsenal category Jun 19, 2023
@LinkIsGrim LinkIsGrim added the kind/feature Release Notes: **ADDED:** label Jun 19, 2023
@YetheSamartaka
Copy link
Contributor

Have you tried this change with your other PR (#9221)? I had difficulties adding more than 10 categories (Seems to be capped at 10, but I'm not entirely sure)

@LinkIsGrim
Copy link
Contributor Author

There's a hard limit on 10 extra categories in the arsenal, yeah. Out of scope for this, but why someone would have more than 10 extra categories is beyond me.

@YetheSamartaka
Copy link
Contributor

YetheSamartaka commented Jun 24, 2023

Well, when this PR and #9221 gets merged, only one will be displayed, because adding just one will reach 10 limit and any newly registered categories will not work. So essentially, your other PR is for nothing because it will never be displayed if this gets merged as well due to reaching that limit.

I like this change and splitting things into categories. In KAM, we have our own Medical category, but your solution is better. However, when you add your other category for Field Rations, it won't work together. I wanted to add a separate category for GreenMag and a couple of other mods as well, so the limit should be addressed when this PR is tampering with the categories in this way.

@LinkIsGrim
Copy link
Contributor Author

Well, when this PR and #9221 gets merged, only one will be displayed, because adding just one will reach 10 limit and any newly registered categories will not work. So essentially, your other PR is for nothing because it will never be displayed if this gets merged as well due to reaching that limit.

I like this change and splitting things into categories. In KAM, we have our own Medical category, but your solution is better. However, when you add your other category for Field Rations, it won't work together. I wanted to add a separate category for GreenMag and a couple of other mods as well, so the limit should be addressed when this PR is tampering with the categories in this way.

Not the case on not doing anything, the categories work in addition to the base ones. Both PRs merged:
20230624180552_1

Category handling is out of scope for this one, and I don't see a good way to do it (adding a scroll bar there would be hell).

@YetheSamartaka
Copy link
Contributor

Well, when this PR and #9221 gets merged, only one will be displayed, because adding just one will reach 10 limit and any newly registered categories will not work. So essentially, your other PR is for nothing because it will never be displayed if this gets merged as well due to reaching that limit.
I like this change and splitting things into categories. In KAM, we have our own Medical category, but your solution is better. However, when you add your other category for Field Rations, it won't work together. I wanted to add a separate category for GreenMag and a couple of other mods as well, so the limit should be addressed when this PR is tampering with the categories in this way.

Not the case on not doing anything, the categories work in addition to the base ones. Both PRs merged: 20230624180552_1

Category handling is out of scope for this one, and I don't see a good way to do it (adding a scroll bar there would be hell).

I see. Then I must have made some kind of mistake when I was adding new category using the same way and it was not working for me (At that time, I only had 2 categories added as an extra). Thank you for testing it out together.

@jonpas jonpas added this to the 3.16.0 milestone Jun 24, 2023
@veteran29
Copy link
Member

Category handling is out of scope for this one, and I don't see a good way to do it (adding a scroll bar there would be hell).

Excess categories could be hidden under another "category" button (3 dots or something like in mobile apps) that shows overlay menu with them temporarily after being clicked.

_list pushBack (configName _x);
} forEach (_fnc_isMedicalItem configClasses (configFile >> "CfgWeapons"));

uiNamespace setVariable [QGVAR(treatmentItems), compileFinal str (_list arrayIntersect _list)]
Copy link
Member

Choose a reason for hiding this comment

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

Why are you storing code? I don't like this, this should store an array that gets cache-invalidated as necessary. Not sure how other things in Arsenal are done though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #9220 (comment), though I don't see a reason we need to protect this.

Copy link
Member

Choose a reason for hiding this comment

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

Do we do that for other variables in ace_arsenal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope.

Copy link
Member

Choose a reason for hiding this comment

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

Then I really don't see the reason. @PabstMirror ?

Copy link
Contributor

Choose a reason for hiding this comment

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

this is how we've been storing things computed at prestart
see https://github.com/acemod/ACE3/blob/master/addons/cargo/XEH_preStart.sqf

Copy link
Contributor Author

@LinkIsGrim LinkIsGrim Jun 28, 2023

Choose a reason for hiding this comment

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

I can swing either way, but arsenal just leaves things unprotected (performance concerns, I guess). I guess once 2.14 compileFinal hits this can be switched to a proper data type anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Release Notes: **ADDED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants