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 - Add better support for status effects in other components #10468

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

Conversation

DartRuffian
Copy link
Contributor

@DartRuffian DartRuffian commented Oct 31, 2024

When merged this pull request will:

  • Add prefixes to status effects, i.e. "blockDamage" -> QGVAR(blockDamage)
    • BWC for all current status effects
    • Currently status effects added in another component always have ace_common_ appended to it, see #10462
  • Fix small capitalization mistakes in comments

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

@@ -29,21 +29,20 @@ if (isNull _object) exitWith {};
//We only do anything if the effect has been defined at some point in the game for this unit
TRACE_2("checking if event is nil",_x,_effectNumber);
if (_effectNumber != -1) then {
private _eventName = format [QGVAR(%1), _x];
Copy link
Contributor

Choose a reason for hiding this comment

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

Anyone using the old format will now be broken

["myThing", true, []] call ace_common_fnc_statusEffect_addType;
["ace_common_myThing", {systemChat str _this}] call CBA_fnc_addEVentHandler;
[player, "myThing", "reason", true] call ace_common_fnc_statusEffect_set;

nothing happens

Copy link
Contributor Author

@DartRuffian DartRuffian Oct 31, 2024

Choose a reason for hiding this comment

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

Adding status effects isn't public API, only get/set are

If something was intentionally adding ace_common_ to a name, I feel that's pretty clear it's only meant to be internally used

Copy link
Contributor

@PabstMirror PabstMirror left a comment

Choose a reason for hiding this comment

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

I think these changes could break things
and just using QEGVAR(common,field_rations_blockUpdates) is not that bad

@DartRuffian
Copy link
Contributor Author

I think these changes could break things and just using QEGVAR(common,field_rations_blockUpdates) is not that bad

True, but it makes weird naming conventions. It also doesn't make a lot of sense to someone reading through the code until they get to addType and see that the event gets ace_common_ added to the name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants