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

Add localUnits array and use that inside medical statemachines #4836

Merged
merged 19 commits into from
Feb 27, 2019

Conversation

dedmen
Copy link
Contributor

@dedmen dedmen commented Jan 17, 2017

This will add an array of localUnits which is kept up-to-date by Eventhandlers and remove the need to dynamically create that list every few frames inside the medical state machines.

The EH has about a performance penalty of 0.015ms per spawned unit.

Currently the Statemachines inside medical_ai and medical_blood create this array of local Units dynamically each time they refresh their list (https://github.com/CBATeam/CBA_A3/blob/master/addons/statemachine/fnc_clockwork.sqf#L37)
by using allUnits select {local _x} which has about a 0.24ms impact.

This is basically moving the performance impact from a PFH thats executed every few frames to an Eventhandler and also reducing the performance cost in the meantime.

I didn't test this yet but i don't know what could go wrong. Besides the "local" EH misbehaving.

Currently the Statemachine clockwork for the 2 medical Statemachines is with 1.2ms the longest executing PFH when on a server with exactly 100 non-local AI's.
diag_captureFrame:
diag_captureFrame PFH
And because 0 of these AI's are local and the Statemachine will recreate the unit list when it iterated through it fully. It will recreate it every frame.
For both Statemachines thats about 0.48ms per frame just for recreating that list over and over again.
This PR should cut down on the bulk of that.

This would essentially be replaced by BI adding a localUnits script function. Which i would hope they would do but.. They probably won't.

@PabstMirror PabstMirror added the kind/enhancement Release Notes: **IMPROVED:** label Jan 17, 2017
@PabstMirror PabstMirror added this to the 3.9.0 milestone Jan 17, 2017
@PabstMirror
Copy link
Contributor

I think this is a good idea, will really shine with high unit counts.

Need to use retro init - }, true, [], true] call CBA_fnc_addClassEventHandler;
Or nothing gets added at start like player.
But that's currently bugged - CBATeam/CBA_A3#567

Need to handle deleted units, Deleted EH is a good canidate but it's for 1.68.

@dedmen
Copy link
Contributor Author

dedmen commented Jan 17, 2017

A quick workaround against CBATeam/CBA_A3#567 would probably just be using pushBackUnique
What about adding the EH's at preInit? That was actually what commy suggested. preInit should be before unit spawns right?

@PabstMirror
Copy link
Contributor

Adding at preInit or just using the config should work fine.

@BaerMitUmlaut
Copy link
Member

@PabstMirror: deleted units should become objectNull, right? The state machine skips null elements.

@PabstMirror
Copy link
Contributor

Think of something like ALIVE that does unit caching
Constantly creating and deleting units
The list would be constantly growing
We would waste more time filtering though a list of a 1000 objNulls

@dedmen
Copy link
Contributor Author

dedmen commented Jan 18, 2017

Then we should rather wait till 1.68 with this.
For the EH's there are three possibilities

  • Cram all the code inside the config EH
  • Create a seperate function for each EH and call that from the config
  • Add in preInit.

I do favor the preInit one if thats reliable enough. What do you think?

@commy2
Copy link
Contributor

commy2 commented Jan 18, 2017

Solution for deleted units is using the "deleted" event handler (requires 1.68)
Solution for killed units is using the "killed" event handler.

I think these two should cover these potential problems.

@commy2
Copy link
Contributor

commy2 commented Jan 18, 2017

Retro init is not needed if the "init" event handler is installed in preInit. (Which should be the way to go anyways).

@commy2
Copy link
Contributor

commy2 commented Jan 18, 2017

Since the state machine is filtering null objects anyway, I think we can just add the "deleted" event handler and play the waiting game with the Arma update. CBA will update soon, hopefully.

@PabstMirror PabstMirror modified the milestones: Ongoing, 3.9.0 Jan 19, 2017
@PabstMirror PabstMirror self-assigned this May 1, 2017
@PabstMirror PabstMirror changed the base branch from master to medical-rewrite May 1, 2017 18:22
@PabstMirror PabstMirror changed the base branch from medical-rewrite to master May 1, 2017 18:22
@PabstMirror
Copy link
Contributor

image

It's failing when deleting via zeus

// Debug code:
[{isNull _this}, {diag_log text format ["%1 - %2 became null", diag_frameNo, this]}, cursorObject] call CBA_fnc_waitUntilAndExecute;

// Deleting via deleteVehicle
45596 unit deleted: _unit=10a29ec40# 164025: b_soldier_01.p3d, local _unit=true z\ace\addons\common\functions\fnc_setupLocalUnitsHandler.sqf:56
45597 unit deleted nextFrame: _unit=<NULL-object>, local _unit=false, isNull _unit=true z\ace\addons\common\functions\fnc_setupLocalUnitsHandler.sqf:61
45597 - any became null

// Deleting via zeus
53311 unit deleted: _unit=10a2bef40# 164031: b_soldier_01.p3d, local _unit=true z\ace\addons\common\functions\fnc_setupLocalUnitsHandler.sqf:56
Info: Zeus interface queued deletion of Alpha 2-2 group with 0 units inside.
53312 unit deleted nextFrame: _unit=10a2bef40# 164031: b_soldier_01.p3d, local _unit=true, isNull _unit=false z\ace\addons\common\functions\fnc_setupLocalUnitsHandler.sqf:61
53313 - any became null

So it takes 2+ frames for it to actually be deleted?

@dedmen
Copy link
Contributor Author

dedmen commented Nov 7, 2017

🤕 I hate Arma.
It can still happen that we have null objects in localUnits array.. In fact it will happen.
CBA's nextFrame executes after the PFH's.
So if unit get's null at start of frame it will still be in localUnits array at the PFH's.
It will most likely get deleted in the Simulation cycle and Draw3D runs after the simulation cycle.. So using Draw3D could get around that.

I guess easiest solution might just be having a PFH in Draw3D that does
localUnits = localUnits - [objNull]
Does the vanilla deletedEH really get called when spawning vehicles in Zeus? Could it be just a XEH issue?

@dedmen
Copy link
Contributor Author

dedmen commented Nov 20, 2017

In the end.. Can't we just make a getLocalUnits function.
That does localUnits = localUnits - [objNull] on the first call every frame?
Perf wise it's crap. But still a lot better than everyone doing allUnits select {local _x}
And if someone doesn't care about null objects they can just use the variable directly.

Then we should also remove the deleted EH as it only creates problems.
@PabstMirror thoughts? I don't see problems with this solution.

Btw I figured out why Zeus deletion takes 2 frames.
A frame is like this
<framestart><scheduled scripts and PFH><simulation><UI scripts and draw3D><frameend>

deleting a vehicle schedules it's deletion. And it get's really deleted in next simulation.

so call deleteVehicle from scheduled or PFH. After that simulation runs and deletes vehicle. Next frame in PFH we check if it was deleted and it was.

Zeus seems to schedule deletion in simulation or UI cycle.
We get deleted eh and want to check next frame.
We check next frame, simulation didn't happen yet so unit is still !null. In the UI cycle it is null but we only notice on next PFH.
So it looks like we are 2 frames late. But actually we are only
frameend,framestart,pfh,simulation,draw3d,frameend,framestart late. As you can see one simulation in there.

Normal deleteVehicle looks like
simulation,draw3d,frameend,framestart

params ["_unit"];
TRACE_3("unit deleted nextFrame",_unit,local _unit,isNull _unit);
if (isNull _unit) then { //If it is not null then the deleted EH was Fake.
GVAR(localUnits) = GVAR(localUnits) - [objNull];
Copy link
Contributor Author

@dedmen dedmen Nov 20, 2017

Choose a reason for hiding this comment

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

I delete all objNull here as using find to find a null object will find the first null object and not the one the deleted EH belongs to anyway.
I would prefer to just call FUNC(getLocalUnits) here in case we add a check to getLocalUnits so it only removes null objects once per frame.
But it would hurt readability.

*/

//Remove null objects
GVAR(localUnits) = GVAR(localUnits) - [objNull];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can check diag_frameNo against a global variable so we only remove null objects once a frame to get us a little more perf.
But I'm not sure if GVAR(localUnits) = GVAR(localUnits) - [objNull]; is really that expensive that it's worth it. Needs profiling with atleast a hundred local units.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually we only need to run this if there was a deletedEH in the last 2-3 frames. But I don't think it hurts.
Considering this will already be about a 100x improvement vs iterating through allUnits everytime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also to checking diag_frameNo objects are deleted in simulation cycle. Draw3D runs after that in the same frame. So that could cause Draw3D to still get null objects.

};
}] call CBA_fnc_addClassEventHandler;

["CAManBase", "respawn", {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't "init" also fire for respawning units?
But I guess they might be !alive at init?

Respawning in MP should be tested.

Copy link
Contributor

Choose a reason for hiding this comment

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

Init does not fire for respawned objects.

@dedmen
Copy link
Contributor Author

dedmen commented Mar 1, 2018

What's currently the problem? Besides merge conflict.
The problem with Zeus deleting units that are not deleted could be solved by using a draw3D EH 1 frame after Zeus "deletes" the unit. We don't have a execNextFrame utility function for that though. And the EH fires in simulation cycle. Meaning right after that is a draw3D EH which we don't want. So if we add in the sim cycle we first have to skip one run.

As vehicles are deleted in SIM cycle this means after using Draw3D it's "impossible" to get null objects into the localUnits array. Because we clean up right after they get deleted.
But Draw3D handlers that run before our's could still see null objects.
Others already said null-objects aren't a big problem. As they can appear in allUnits anyway.

@PabstMirror
Copy link
Contributor

Moved isBleeding check to inside state machine as checking FUNC(isBleeding) for all units was still expensive. Even with high unit count I think the delay between bleeding is high enough that this should not cause issues.

image

@PabstMirror PabstMirror merged commit 7f04d00 into acemod:master Feb 27, 2019
@dedmen
Copy link
Contributor Author

dedmen commented Feb 27, 2019

What about the issues we talked about?

@PabstMirror
Copy link
Contributor

I tested on a dedicated server and it and
call ace_common_fnc_getLocalUnits matched allUnits select {local _x}

The addition of GVAR(localUnits) = GVAR(localUnits) - [objNull]; in the getter fixes any problems with deletedEH. I guess we could even remove the deletedEH as it still is unreliable with zeus

@PabstMirror
Copy link
Contributor

["CAManBase", "deleted", {
    params ["_unit"];
    TRACE_2("AAA unit deleted",_unit,local _unit);

    if (local _unit) then {
        [{
            params ["_unit"];
            TRACE_3("BBB unit deleted nextFrame",_unit,local _unit,isNull _unit);
            [{
                params ["_unit"];
                TRACE_3("CCC unit deleted next-nextFrame",_unit,local _unit,isNull _unit);
                if (isNull _unit) then { // If it is not null then the deleted EH was Fake.
                    GVAR(localUnits) = GVAR(localUnits) - [objNull];
                };
            }, _this] call CBA_fnc_execNextFrame;
        }, [_unit]] call CBA_fnc_execNextFrame;
    };
}] call CBA_fnc_addClassEventHandler;

this should also work

PabstMirror added a commit that referenced this pull request Mar 26, 2019
BaerMitUmlaut pushed a commit that referenced this pull request Aug 5, 2019
* Added localUnits EH

Code by @commy2

* Init localUnits Variable

* use localUnits for medical_blood statemachine

* use localUnits for medical_ai statemachine

* Remove bracket hell

* Add Deleted EH

* Run at preInit, move to seperate file

* Handle Respawns, Filter Dead

* Add detection for Fake deleted EH

* Use a getter function

* delete all objNull at deletedEH

* Cleanup header/comments

* Move isBleeding check to inside statemachine

* opps

* debug off
BaerMitUmlaut pushed a commit that referenced this pull request Aug 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Release Notes: **IMPROVED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants