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

Interact menu - Fix condition in consolidated menu #9946

Conversation

Dystopian
Copy link
Contributor

When merged this pull request will:

  • title.

Currently consolidated menu runs original condition code before statement but with child custom params. This can lead to error when child params are different to parent params.

Thanks @mrschick for another pointing to problem in #9876.

_insertChildrenChild changes are made just for consistency. In fact it can be replaced with {} because it looks like it's never used after fnc_collectActiveActionTree.

I don't know why this wasn't implemented in #8060 :-(

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.

lgtm?

@LinkIsGrim LinkIsGrim added the kind/bug-fix Release Notes: **FIXED:** label Apr 15, 2024
@PabstMirror
Copy link
Contributor

test code

_condition = {
    systemChat format ["C1 Cond %1",_this#2];
    true
};
_statement = {};
_insert = {
    _subAction = ["C2","C2","",{systemChat format ["C2 %1",_this#2];},{systemChat format ["C2 Cond %1",_this#2]; true}, {}, ["222"]] call ace_interact_menu_fnc_createAction;
    [[_subAction, [], player]]
};
_action = ["C1","C1","",_statement,_condition, _insert, ["111"]] call ace_interact_menu_fnc_createAction;
[(typeOf player), 1, ["ACE_SelfActions"], _action] call ace_interact_menu_fnc_addActionToClass;

prints out C1 Cond ["222"]
and this PR fixes the issue

@PabstMirror PabstMirror added this to the 3.17.1 milestone Apr 25, 2024
@PabstMirror PabstMirror merged commit a12ad9e into acemod:master Apr 25, 2024
5 checks passed
@PabstMirror
Copy link
Contributor

a similar bug also exists with reordering
https://github.com/acemod/ACE3/blob/master/addons/interact_menu/functions/fnc_initMenuReorder.sqf#L32-L36
private _conditionFullString = format ["%1 && {%2}", _parentConditionString, _condition call EFUNC(common,codeToString)];
will call child and parent with the child's params
but I really don't see this being a problem because params are almost exclusively used with dynamic children and wouldn't be show as re-orderable, I think?

@Dystopian Dystopian deleted the fix-consolidated-interaction-menu-condition branch April 26, 2024 21:57
@Dystopian
Copy link
Contributor Author

params are almost exclusively used with dynamic children and wouldn't be show as re-orderable

I found only 2 related self-actions:

  1. Artillery Tables

    [ace_player, 1, ["ACE_SelfActions", "ACE_Equipment"], _action] call EFUNC(interact_menu,addActionToObject);

    It uses Equipment as parent which has true condition:
    class ACE_Equipment {
    displayName = CSTRING(Equipment);
    condition = QUOTE(true);

    Also these actions are loaded after settings created so no problem here at all.

  2. Medical GUI

    GVAR(selfInteractionActions) pushBack ["", 1, ["ACE_SelfActions", "ACE_Medical", _actionPath], _action];

    which uses Medical parent with setting in condition
    class ACE_Medical {
    displayName = CSTRING(Medical);
    condition = QGVAR(enableSelfActions);

    and true in subactions
    #define ACTION_CONDITION condition = "true";

So yes I also don't see a problem with reordering.

But I found a bug: reordered Equipment menu has no artillery tables. I'm working on it.

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

Successfully merging this pull request may close these issues.

3 participants