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

Added: QTE Framework #1649

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

Conversation

john681611
Copy link

When merged this pull request will:

@john681611 john681611 marked this pull request as draft March 19, 2024 23:42
addons/common/fnc_generateQTESequence.sqf Outdated Show resolved Hide resolved
Comment on lines 32 to 35
if (!GVAR(settingsInitFinished)) exitWith {
// only run this after the settings are initialized
GVAR(runAtSettingsInitialized) pushBack [FUNC(runQTE), _this];
};
Copy link
Contributor

Choose a reason for hiding this comment

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

For now leave this aside, or bring it up-to-date with CBA.

Comment on lines 38 to 54
TRACE_1("QTE already running qeueing up", GVAR(QTERunning));
[{
!GVAR(QTERunning)
}, {
_this call FUNC(runQTE);
}, _this] call CBA_fnc_waitUntilAndExecute;
};

private _display = findDisplay 46;

if (isNull _display) exitWith {
TRACE_1("Waiting for main display to be ready", isNull (_display));
[{
!isNull (findDisplay 46)
}, {
_this call FUNC(runQTE);
}, _this] call CBA_fnc_waitUntilAndExecute;
Copy link
Contributor

Choose a reason for hiding this comment

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

Imo it should be up to the user to check whenever it's ready. This should only return false or an error code that it couldn't run. The removal of these waitUntilAndExecute would make the _onDisplay event redundant.

The display on which the QTE is drawn should be an argument, so that people can run it wherever they want to (e.g. Zeus interface). Because of comment below, this is no longer relevant.

Comment on lines 82 to 96
private _inputKeys = [DIK_UP, DIK_DOWN, DIK_LEFT, DIK_RIGHT];
switch (GVAR(QTEInputKeys)) do {
case 1: {
_inputKeys = [DIK_W, DIK_S, DIK_A, DIK_D];
};
case 2: {
_inputKeys = [DIK_I, DIK_K, DIK_J, DIK_L];
};
case 3: {
_inputKeys = [DIK_NUMPAD8, DIK_NUMPAD2, DIK_NUMPAD4, DIK_NUMPAD6];
};
default {
_inputKeys = [DIK_UP, DIK_DOWN, DIK_LEFT, DIK_RIGHT];
};
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I really dislike this strongly. It uses CBA settings for keybind sets, when we can use CBA keybinds instead.
This would allow for way more flexibility and customisability, as 3rd party mods or mod makers could add their own extra keybinds.

Register 4 keybinds. Here's an example (strings would need to be localised and improved):

["QTE Keybinds", QGVAR(keyUpQTE), ["QTE up key", "Up key used in QTE events."], {
    [""] call FUNC(keyPressedQTE); // return
}, {}, [DIK_UP, [false, false, false]]] call CBA_fnc_addKeybind;

In fnc_keyPressedQTE, you'd check if the keypress is good or not:

params ["_eventQTE"];

// Check if the passed parameter is the next key in the QTE sequence
// If yes, continue
// If no, stop

Imo, we should make the QTE agonistic of the QTE chars themselves ("↑"). That should be configurable by the user. It will add an extra layer of complexity to the whole things, so we'll have to see if it's worth it.

Copy link
Author

@john681611 john681611 Mar 21, 2024

Choose a reason for hiding this comment

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

Let's keep it simple and start with assigning keys to "↑" etc then we can look at expanding to other keys.

@john681611
Copy link
Author

john681611 commented Mar 23, 2024

@johnb432 Only thing I've not managed to get working is the keybinds actually calling the function
Ok defaulting my keybinds worked

@john681611 john681611 marked this pull request as ready for review March 23, 2024 20:11
@@ -78,3 +79,19 @@ activateAddons GVAR(addons);
["CAManBase", "InitPost", CBA_fnc_randomizeFacewear] call CBA_fnc_addClassEventHandler;

ADDON = true;

["CBA QTE", "QTE_Up_Key", ["↑", "Up key used in QTE events."], {}, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally have a problem with QTE events cuz it expands into Quick-Time Event events.

Making it Quick-Time Events ain't just 2 letters longer, I admit, but shouldn't matter:

Up key used in QTE events.
Up key used in Quick-Time Events.

* [["↑", "↓", "→", "←"]] call ace_common_fnc_getFormattedQTESequence
*
* Public: Yes
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

The other headers are formatted different from this one.

};

if (_onDisplay isEqualType "") then {
[_onDisplay, [_args, _qte_seqence, GVAR(QTEHistory)]] call CBA_fnc_localEvent;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[_onDisplay, [_args, _qte_seqence, GVAR(QTEHistory)]] call CBA_fnc_localEvent;
[_onDisplay, [_args, _qte_sequence, GVAR(QTEHistory)]] call CBA_fnc_localEvent;

if (_onDisplay isEqualType "") then {
[_onDisplay, [_args, _qte_seqence, GVAR(QTEHistory)]] call CBA_fnc_localEvent;
} else {
[_args, _qte_seqence, GVAR(QTEHistory)] call _onDisplay;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[_args, _qte_seqence, GVAR(QTEHistory)] call _onDisplay;
[_args, _qte_sequence, GVAR(QTEHistory)] call _onDisplay;

_onDisplay - Code callback on displayable event passed [_args, _qte_seqence, _qte_history]. <CODE, STRING>
_onFinish - Code callback on QTE completed passed [_args, _elapsedTime]. <CODE, STRING>
_onFinish - Code callback on QTE timeout/outranged passed [_args, _elapsedTime]. <CODE, STRING>
_qte_seqence - QTE seqence usually made up of ["↑", "↓", "→", "←"] <ARRAY>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_qte_seqence - QTE seqence usually made up of ["", "", "", ""] <ARRAY>
_qte_seqence - QTE sequence usually made up of ["", "", "", ""] <ARRAY>


Example:
[
car,
Copy link
Contributor

Choose a reason for hiding this comment

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

A lot of tabs in this file at least.

[5] call CBA_fnc_generateQTESequence;

Returns:
QTE seqence of requested length made up of ["↑", "↓", "→", "←"] <ARRAY>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
QTE seqence of requested length made up of ["", "", "", ""] <ARRAY>
QTE sequence of requested length made up of ["", "", "", ""] <ARRAY>

private _onFinish = GVAR(QTEArgs) get "onFinish";
private _onFail = GVAR(QTEArgs) get "onFail";
private _max_distance = GVAR(QTEArgs) get "max_distance";
private _qte_seqence = GVAR(QTEArgs) get "qte_seqence";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private _qte_seqence = GVAR(QTEArgs) get "qte_seqence";
private _qte_sequence = GVAR(QTEArgs) get "qte_sequence";

GVAR(QTEHistory) pushBack _eventQTE;


if (GVAR(QTEHistory) isEqualTo _qte_seqence) exitWith {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (GVAR(QTEHistory) isEqualTo _qte_seqence) exitWith {
if (GVAR(QTEHistory) isEqualTo _qte_sequence) exitWith {

};
};

if !(GVAR(QTEHistory) isEqualTo (_qte_seqence select [0, count GVAR(QTEHistory)])) then {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if !(GVAR(QTEHistory) isEqualTo (_qte_seqence select [0, count GVAR(QTEHistory)])) then {
if !(GVAR(QTEHistory) isEqualTo (_qte_sequence select [0, count GVAR(QTEHistory)])) then {

Parameters:
_object - <OBJECT>
_args - Extra arguments passed to the _on... functions<ARRAY>
_onDisplay - Code callback on displayable event passed [_args, _qte_seqence, _qte_history]. <CODE, STRING>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_onDisplay - Code callback on displayable event passed [_args, _qte_seqence, _qte_history]. <CODE, STRING>
_onDisplay - Code callback on displayable event passed [_args, _qte_sequence, _qte_history]. <CODE, STRING>

Copy link
Contributor

@johnb432 johnb432 left a comment

Choose a reason for hiding this comment

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

I haven't reviewed the functionality yet, this is mostly formatting related.

addons/common/XEH_preInit.sqf Outdated Show resolved Hide resolved
addons/common/fnc_generateQTESequence.sqf Outdated Show resolved Hide resolved
addons/common/fnc_keyPressedQTE.sqf Outdated Show resolved Hide resolved
Process Quick-Time Key Press

Parameters:
_eventQTE - <STRING>
Copy link
Contributor

Choose a reason for hiding this comment

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

Argument needs description.

["↑"] call CBA_fnc_keyPressedQTE;

Returns:
Nil
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be None, but the whole point of using a function over an event is so that you can return something.

We want to "override" input if it's a valid QTE keypress, meaning that e.g. if you have bound your arrow up key to move forwards, you wouldn't move forwards while you are doing a QTE.

Copy link
Author

Choose a reason for hiding this comment

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

Is this where we return true/false in terms of input override?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep

Comment on lines 46 to 51
TRACE_1("QTE already running qeueing up",GVAR(QTERunning));
[{
!GVAR(QTERunning)
}, {
_this call FUNC(runQTE);
}, _this] call CBA_fnc_waitUntilAndExecute;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still not a fan of this. Depending, we might have to wait indefinitely. It also means that multiple QTE can queue up, which could cause unforeseen headaches, albeit unlikely.

I think we'd be better off returning a boolean telling the caller if the QTE was successfully run or not.

["start_time", _start_time],
["timeout", _timeout]
];
GVAR(QTEArgs) = createHashMapObject [_qteArgsArray];
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell, there is no reason to use a hashmap object.

Suggested change
GVAR(QTEArgs) = createHashMapObject [_qteArgsArray];
GVAR(QTEArgs) = createHashMapFromArray [_qteArgsArray];

addons/common/fnc_runQTE.sqf Outdated Show resolved Hide resolved
addons/common/stringtable.xml Outdated Show resolved Hide resolved
} else {
[_args, _elapsedTime] call _onFail;
};
}, _this] call CBA_fnc_waitUntilAndExecute;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can't pass _this when using default arguments. The default arguments are not inserted into _this, you'll have to pass each argument individually in an array.

Regardless, _this here does not make sense. Pass [_object, _start_time, _timeout, _max_distance] and read those in the loop instead - unless reading from GVAR(QTEArgs) is faster?

Copy link
Contributor

@rautamiekka rautamiekka left a comment

Choose a reason for hiding this comment

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

  1. Most files use tabs instead of spaces.

  2. The fnc headers are different from the ones in use. For ex: 698aee1

  3. Apart from a couple extra tabs/spaces I added a suggestion for, lgtm.

Comment on lines 59 to 61
[_onDisplay, [_args, _qte_sequence, GVAR(QTEHistory)]] call CBA_fnc_localEvent;
} else {
[_args, _qte_sequence, GVAR(QTEHistory)] call _onDisplay;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[_onDisplay, [_args, _qte_sequence, GVAR(QTEHistory)]] call CBA_fnc_localEvent;
} else {
[_args, _qte_sequence, GVAR(QTEHistory)] call _onDisplay;
[_onDisplay, [_args, _qte_sequence, GVAR(QTEHistory)]] call CBA_fnc_localEvent;
} else {
[_args, _qte_sequence, GVAR(QTEHistory)] call _onDisplay;

@john681611
Copy link
Author

Is there a linter in the repo I can run or format config just to save time on these formatting comments?

@johnb432
Copy link
Contributor

Is there a linter in the repo I can run or format config just to save time on these formatting comments?

No, I don't think so.

addons/common/fnc_runQTE.sqf Outdated Show resolved Hide resolved
addons/common/fnc_runQTE.sqf Outdated Show resolved Hide resolved
john681611 and others added 2 commits April 2, 2024 18:24
addons/common/fnc_runQTE.sqf Outdated Show resolved Hide resolved
Copy link
Contributor

@rautamiekka rautamiekka left a comment

Choose a reason for hiding this comment

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

As far as I can tell by eyesight everything's good, but don't just take my word for it.

@john681611
Copy link
Author

Thanks for the reviews guys sorry its 90% formatting issues.

@@ -78,3 +79,19 @@ activateAddons GVAR(addons);
["CAManBase", "InitPost", CBA_fnc_randomizeFacewear] call CBA_fnc_addClassEventHandler;

ADDON = true;
Copy link
Contributor

@johnb432 johnb432 Apr 30, 2024

Choose a reason for hiding this comment

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

All the code should be added before this line.

However, more importantly adding keybinds in common doesn't work. keybinding depends on common, meaning the code below is called when CBA_fnc_addKeybind is not defined.

This means that this code needs to be moved to another component. I'm not sure if events is the right place or if we should add another component, just for this.

addons/common/XEH_preInit.sqf Outdated Show resolved Hide resolved
Copy link
Contributor

@johnb432 johnb432 left a comment

Choose a reason for hiding this comment

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

It's looking pretty good, but some changes are needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need this file, remove it.

Comment on lines 1 to 6
class Extended_PreStart_EventHandlers {
class ADDON {
init = QUOTE(call COMPILE_SCRIPT(XEH_preStart));
};
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove along with the file.

Comment on lines 6 to 7
PATHTO_FNC(runQTE);
PATHTO_FNC(keyPressedQTE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Order them alphabetically.

Suggested change
PATHTO_FNC(runQTE);
PATHTO_FNC(keyPressedQTE);
PATHTO_FNC(keyPressedQTE);
PATHTO_FNC(runQTE);

Comment on lines 45 to 47
if (GVAR(QTERunning)) exitWith {
false
};
Copy link
Contributor

Choose a reason for hiding this comment

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

GVAR(QTERunning) is undefined on the first time it's run.

Suggested change
if (GVAR(QTERunning)) exitWith {
false
};
if !(missionNamespace getVariable [QGVAR(QTERunning), false]) exitWith {
false
};

You could move this line above the params line, as this doesn't require any arguments.

["onFinish", _onFinish],
["onFail", _onFail],
["maxDistance", _maxDistance],
["qteSeqence", _qteSequence],
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor typo has slipped in (change where necessary).

Suggested change
["qteSeqence", _qteSequence],
["qteSequence", _qteSequence],

Comment on lines 78 to 79
private _onFail = (GVAR(QTEArgs) get "onFail");
private _args = (GVAR(QTEArgs) get "args");
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary parenthesis:

Suggested change
private _onFail = (GVAR(QTEArgs) get "onFail");
private _args = (GVAR(QTEArgs) get "args");
private _onFail = GVAR(QTEArgs) get "onFail";
private _args = GVAR(QTEArgs) get "args";


GVAR(QTEHistory) pushBack _eventQTE;


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Check if the input corresponds to the sequence

};
true
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// If the user failed an input, wipe the previous input from memory

private _maxDistance = GVAR(QTEArgs) get "maxDistance";
private _elapsedTime = CBA_missionTime - (GVAR(QTEArgs) get "startTime");

!GVAR(QTERunning) || player distance _object > _maxDistance || _elapsedTime > _timeout;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think having player distance _object > _maxDistance as a condition is a very limiting factor. In some cases one might not want or need to have such a condition, but would rather use others.
The condition code should be passed by the user, it would be {false} by default (meaning the timeout would be the default method of ending the QTE if it isn't solved in time). object and maxDistance would no longer be needed.

You'd evaluate it by calling _args call _condition, with _args being the same args as the event/function one.
The function header would need to be adjusted accordingly.

Copy link
Author

Choose a reason for hiding this comment

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

Why not go a step further and remove the timeout and just pass [_args, _elapsedTime]. This would allow for unlimited time variants too. Its also standard for all functions then.

---------------------------------------------------------------------------- */


params ["_object", "_args", "_onDisplay", "_onFinish", "_onFail", "_qteSequence", ["_maxDistance", 10], ["_timeout", 30]];
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a public function, we need to sanitise the input:

Suggested change
params ["_object", "_args", "_onDisplay", "_onFinish", "_onFail", "_qteSequence", ["_maxDistance", 10], ["_timeout", 30]];
params [["_object", objNull, [objNull]], "_args", ["_onDisplay", {}, ["", {}]], ["_onFinish", {}, ["", {}]], ["_onFail", {}, ["", {}]], ["_qteSequence", [], [[]]], ["_maxDistance", 10, [0]], ["_timeout", 30, [0]]];

You need to check if the input are valid (e.g. an empty QTE sequence should not be allowed).

I haven't taken into account the other proposals I've made, so you'll need to adapt accordingly.

@john681611
Copy link
Author

Decided to go down the maximum flexibility route. We will let users define their conditions of failure entirely. I'm also passing a reset count so attempts can be implemented.

@johnb432
Copy link
Contributor

johnb432 commented May 9, 2024

Quick glance over changes: Looks good, but I'll take a more detailed look later.

@john681611
Copy link
Author

Is there any progress on this?

@veteran29 veteran29 self-requested a review June 4, 2024 17:59
@Mike-MF
Copy link
Contributor

Mike-MF commented Jun 23, 2024

Aside from the typo I saw, I played around with this. It's really nice, I like the configuration aspects to it too. I did a bunch of things with it and couldn't find a way to break it.

Nice job.

Copy link
Contributor

@johnb432 johnb432 left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

None yet

5 participants