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

SearchNearby Endless Loop #1544 #1545

Closed
wants to merge 7 commits into from
Closed

SearchNearby Endless Loop #1544 #1545

wants to merge 7 commits into from

Conversation

mrzachhigginsofficial
Copy link
Contributor

When merged this pull request will:

  • Performance and reliability adjustments for SearchNearby function
  • Set a max timeout for each unit move order (positions *15 and no more than 2 minutes).
  • Set a time start variable "CBASearchTime" on each unit.
  • Added a quarter-second pause between unit iterations.
  • If the timeout is exceeded before the unit is ready, the unit will receive a new command for the next position. Command time is updated to reflect timeout for the next move command.

@mrzachhigginsofficial
Copy link
Contributor Author

Proposed PR for issue #1544

Co-authored-by: Jouni Järvinen <rautamiekka@users.noreply.github.com>
Co-authored-by: Jouni Järvinen <rautamiekka@users.noreply.github.com>
Co-authored-by: Jouni Järvinen <rautamiekka@users.noreply.github.com>
@commy2
Copy link
Contributor

commy2 commented May 10, 2022

Added a quarter-second pause between unit iterations.

Why?

Set a time start variable "CBASearchTime" on each unit.

Should not be a hard coded string in a local variable, but a QGVAR, i.e. QGVAR(SearchTimeout)

Not sure about using time. It uses the SQF NUMBER type single precision float, i.e. this change will make the script fail after roughly 11.5 days uptime.

// Create limiters for each unit
private _timeout = (count(_positions) * 15) min 120;
private _timetag = "CBASearchTime";
{_x setVariable [_timetag,time]} forEach units _group;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not below use getVariable default instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The units would eventually skip every building as the start time would never be updated. Perhaps the variable name is confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps QGVAR(StartSearchTime) would make more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in aa63e9a

if (_positions isEqualTo []) exitWith {};
if (unitReady _x) then {
if (unitReady _x || {_starttime + _timeout < time}) then {
Copy link
Contributor

Choose a reason for hiding this comment

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

If lazy eval is used here, may as well inline the whole local variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As such?

if (unitReady _x || {(_x getVariable _timetag) + _timeout < time}) then {...}

@commy2
Copy link
Contributor

commy2 commented May 10, 2022

Timeout should probably be a function parameter instead. Then it is opt in (default it would be off to keep the current behaviour), and tweakable. I would not make it dependent on group size at all.

Co-authored-by: commy2 <commy-2@gmx.de>
@mrzachhigginsofficial
Copy link
Contributor Author

Timeout should probably be a function parameter instead. Then it is opt in (default it would be off to keep the current behaviour), and tweakable. I would not make it dependent on group size at all.

This is not dependent on group size but is dependent on the number of positions in the chosen building. Unit timeouts are evaluated the same whether there are 2 or 100.

private _positions = _building buildingPos -1;
...
private _timeout = (count _positions * 15) min 120;

@mrzachhigginsofficial
Copy link
Contributor Author

mrzachhigginsofficial commented May 10, 2022

Regarding parameterizing the function, I agree and propose the following parameters:

_timeoutcoef
_maxtimeout

private _positions = _building buildingPos -1;
_timeout =  (count _positions * _timeoutcoef) min _maxtimeout;

@mrzachhigginsofficial
Copy link
Contributor Author

@commy2 - I need to test that latest commit some more. Please review and let me know if it resolves any of your concerns (especially the opt-out).

@mrzachhigginsofficial
Copy link
Contributor Author

I'm closing this PR for now. The more I dig into this, the more I see more opportunities for these units to get stuck.

Another example in fnc_searchNearby.sqf

    private _wp = _group addWaypoint [getPosASL _leader, -1, currentWaypoint _group];
    private _cond = "({unitReady _x || !(alive _x)} count thisList) == count thisList";
    private _comp = format ["this setFormation '%1'; this setBehaviour '%2'; deleteWaypoint [group this, currentWaypoint (group this)];", formation _group, behaviour _leader];
    _wp setWaypointStatements [_cond, _comp];

Once again, if the units are never ready, this "regroup" waypoint will never complete. I will open a new PR once I have a more comprehensive solution. @commy2 brings up a good point that this should be parameterized as to not change existing functionality, so I will keep that in mind. It is unfortunate so much reliance is put on the AI sim working as intended (especially since it's virtually impossible to account for every scenario).

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.

None yet

3 participants