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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optionals - Add No Medical and No Realistic Names components #8187

Merged
merged 12 commits into from Sep 4, 2023

Conversation

Dystopian
Copy link
Contributor

In some missions ACE Medical can't be used. Also there were some players who wanted to disable Realistic Names component.
I think this PR is most correct way to disable ACE components without mod folder modifying and messing with own mods based on ACE. Empty config is correctly loaded and works as expected when such optional component is enabled.

The biggest problem is that build tool must support __has_include preprocessor command. As of now Mikero tools (DePbo 7.97.0.25) don't support it and even with -B argument they try to rapify/check syntax (fail as result). AddonBuilder builds successfully.

About $NOBIN$. It looks like Mikero tools with -B (Don't binarise) argument do not binarize config.cpp but binarize models etc. AddonBuilder with -packonly argument do not binarize anything. So both tools can't be used for this PR building 馃槥
I would suggest addding new file flag (e.g. $NOBINCONFIG$) to indicate that only config should not be binarized. And naturally build tool (HEMTT?) should support such mode. Config syntax should be checked anyway ignoring __has_include.
@BrettMayson could you implement such feature if ACE team agrees to approve this PR?

Making this PR as draft until there is a tool for building.

@BaerMitUmlaut
Copy link
Member

What's the point of this? What is the advantage of having to add a file instead of having to remove it? This doesn't make sense to me, sorry.

@commy2
Copy link
Contributor

commy2 commented May 7, 2021

Probably something about Steam. I wouldn't know.

@Dystopian
Copy link
Contributor Author

What's the point of this? What is the advantage of having to add a file instead of having to remove it? This doesn't make sense to me, sorry.

  • you don't have to manage files in ACE directory on each ACE update;
  • you can simply disable/enable ACE medical for some game sessions/missions with just loading/unloading optional addon.

@BaerMitUmlaut
Copy link
Member

you don't have to manage files in ACE directory on each ACE update;

Does the optional PBO move itself?

@commy2
Copy link
Contributor

commy2 commented May 9, 2021

No, but you can un/check it in Steam I suppose.

@Dystopian
Copy link
Contributor Author

Does the optional PBO move itself?

You don't have to move optionals since #5038.

@PabstMirror
Copy link
Contributor

Problem we've found is that __has_include only checks if the file can be loaded
but it applies to both the virtual filesystem (files from pbos) and real files in the arma folder
e.g.
#if __has_include("\test\t.hpp")
will return true if you have a "F:\SteamLibrary\steamapps\common\Arma 3\test\t.hpp"

it's just like #inclding a \userconfig\ file
we really need a __has_include that only checks virtual filesystem

@PabstMirror
Copy link
Contributor

preprocessor should respect filepatching in 2.06

@Superxpdude
Copy link

Are there any plans to continue development of this PR? I am interested in this as a feature, and can put in the time to get it working via another PR, but I don't want to duplicate the work in case there are still plans to finish this one up.

Mostly wondering because the limiting factor for this seems to be resolved as far as I can tell based on PR #9116.

@Dystopian
Copy link
Contributor Author

@Superxpdude I will try to update it this week.

@jonpas
Copy link
Member

jonpas commented Jun 19, 2023

What's still preventing Medical from having a setting to enable/disable it?

@Dystopian
Copy link
Contributor Author

What's still preventing Medical from having a setting to enable/disable it?

IIRC there were changes in unit damage config. I didn't find better way to handle it.

@Dystopian Dystopian marked this pull request as ready for review June 23, 2023 04:45
@LinkIsGrim
Copy link
Contributor

LinkIsGrim commented Jun 23, 2023

What's still preventing Medical from having a setting to enable/disable it?

IIRC there were changes in unit damage config. I didn't find better way to handle it.

We could handle the limb armor via script in fnc_handleDamage (ugh), I guess. Will just adding ACE_HDBracket change vanilla behavior in anyway? Scratch that. Need the selections for properly deciding which side took damage. So when are we switching to HitPart (ugh^2)?

addons/fire/addon.toml Outdated Show resolved Hide resolved
optionals/nomedical/$PBOPREFIX$ Outdated Show resolved Hide resolved
optionals/norealisticnames/$PBOPREFIX$ Outdated Show resolved Hide resolved
Comment on lines +3 to +9
#if __has_include("\z\ace\addons\nomedical\script_component.hpp")
#define PATCH_SKIP "No Medical"
#endif

#ifdef PATCH_SKIP
ACE_PATCH_NOT_LOADED(ADDON,PATCH_SKIP)
#else
Copy link
Member

@jonpas jonpas Aug 11, 2023

Choose a reason for hiding this comment

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

Suggested change
#if __has_include("\z\ace\addons\nomedical\script_component.hpp")
#define PATCH_SKIP "No Medical"
#endif
#ifdef PATCH_SKIP
ACE_PATCH_NOT_LOADED(ADDON,PATCH_SKIP)
#else
#if __has_include("\z\ace\addons\nomedical\script_component.hpp")
ACE_PATCH_NOT_LOADED(ADDON,"No Medical")
#else

Why not just this? 馃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This way can define PATCH_SKIP with several conditions. E.g.:

#if __has_include("\z\ace\addons\nomedical\script_component.hpp")
#define PATCH_SKIP "No Medical"
#endif
#if __has_include("\some\other_addon\config.bin")
#define PATCH_SKIP "Some Other Addon"
#endif

and then you use PATCH_SKIP just once to disable component with ACE_PATCH_NOT_LOADED. In this PR this is mostly a good template for other __has_include components. I liked @PabstMirror idea and implemeted it here too.
See also

#if __has_include("\lxWS\data_f_lxWS\config.bin")
#else
#define PATCH_SKIP "Western Sahara"
#endif
#ifdef PATCH_SKIP
ACE_PATCH_NOT_LOADED(ADDON,PATCH_SKIP)

Also you have PATCH_SKIP undefined in suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

Ok your example adds multiple conditions, but you can also do that like this:

#if __has_include("\z\ace\addons\nomedical\script_component.hpp")
#if __has_include("\some\other_addon\config.bin")
ACE_PATCH_NOT_LOADED(ADDON,"Some Other Addon")
#else
ACE_PATCH_NOT_LOADED(ADDON,"No Medical")
#else
// default config
#endif
#endif

Although that is uglier and I wouldn't want to do it.

Not sure, we just need the opposite of skipWhenMissingDependencies!

Fixed suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

I am not fully sure about #if, but I seem to remember that preprocessor "commands" like that are not able to be nested(at least #ifdef is not able to be nested)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, and they also don't support logical operators I believe. Yeah, we need a skipWhenLoaded.

Copy link
Contributor

Choose a reason for hiding this comment

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

#if __has_include("\rhsafrf\addons\rhs_main\loadorder\config.bin")
#else
    #define PATCH_SKIP "RHS AFRF"
#endif

#if __has_include("\z\ace\addons\explosives\script_component.hpp")
#else
    #ifdef PATCH_SKIP
    #else
        #define PATCH_SKIP "ACE Explosives"
    #endif
#endif

works on game, mikro and hemtt

Copy link
Contributor

Choose a reason for hiding this comment

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

What about #ifndef, is it working everywhere ? Cuz if it is, I don't see why do ifdef-else.

Copy link
Member

Choose a reason for hiding this comment

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

but I seem to remember that preprocessor "commands" like that are not able to be nested(at least #ifdef is not able to be nested)

This should be fixed at least for a patch or two.

Copy link
Contributor

Choose a reason for hiding this comment

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

#9294 seems to work fine with nesting.

@jonpas jonpas changed the title Optionals - Add nomedical and norealisticnames components Optionals - Add No Medical and No Realistic Names components Aug 11, 2023
@jonpas jonpas added this to the 3.16.0 milestone Aug 11, 2023
@jonpas jonpas added the kind/feature Release Notes: **ADDED:** label Aug 11, 2023
@LinkIsGrim LinkIsGrim merged commit 10ffcea into acemod:master Sep 4, 2023
5 checks passed
@Dystopian Dystopian deleted the nomedical branch September 5, 2023 15:20
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

10 participants