don't skip next PFH if current one is removed while iterating through #950

commy2 · 2018-07-03T18:16:26Z

When merged this pull request will:

deactivate PFH by replacing the script with {}, then delete NextFrame (guaranteed to happen outside of iterating through the array that is modified)
more elegant than Fix a issue that a PFH could be skip #915 because doesn't change the EachFrame function
close Fix a issue that a PFH could be skip #915

Needs testing, I won't have access to Arma this and the next week.

commy2 · 2018-07-03T18:19:53Z

Q: What happens if the removePFH function is called twice with the same handle in the same frame (deleting already removed PFH waiting to be cleaned up NextFrame)?

dedmen

Code wise looks good. Didn't test tho ._.

dedmen · 2018-07-03T19:42:27Z

addons/common/fnc_removePerFrameHandler.sqf

@@ -29,13 +29,19 @@ if (_handle < 0 || {_handle >= count GVAR(PFHhandles)}) exitWith {};
 [{
    params ["_handle"];

-    GVAR(perFrameHandlerArray) deleteAt (GVAR(PFHhandles) select _handle);
-    GVAR(PFHhandles) set [_handle, nil];
+    GVAR(perFrameHandlerArray) set [GVAR(PFHhandles) select _handle select 0, {}];


Fun fact. call doesn't exit early if it has no code. It will actually really build a callstack and everything required. And then execute the empty code and exit right after.

I wouldn't have implemented it any other way tbh. For simplicity.

dedmen · 2018-07-03T19:44:38Z

addons/common/fnc_removePerFrameHandler.sqf

+        GVAR(PFHhandles) set [_handle, nil];
+
+        {
+            _x params ["", "", "", "", "", "_handle"];


reusing _handle here looks confusing to some people. But these people don't look at CBA code anyway.

(I pressed send review and wanted to submit the comment after that.. But who could've guessed that the comment is discarded when you submit the review.)

I just indented the previous code. The function is simple enough I'd say.

kymckay · 2018-07-03T22:39:05Z

I like the elegance, though running through it in my head I'm also not sure this isn't still broken as per commy's comment above.

The way it is currently, I believe that if the function is called n times in the same frame for the same handler, the next n - 1 handlers would also be deleted.

My first thought:
You'd need to add a sanity check for whether the GVAR(PFHhandles) value is already nil (meaning the handle has already been deleted that frame). However, this would only work assuming the handle index hasn't been refilled by another call to addPerFrameHandler between the duplicated execNextFrame calls (unsure if this can happen, I'll assume it probably can).

My next thought:
It'd be a hell of a lot simpler and actually foolproof to just check if the code is already {} in the scope above before the execNextFrame is even added.

jokoho48 · 2018-07-03T23:12:38Z

I like the base idea over what I have done before. but what I dislike is that you run over every PFH Multiple times when more than 1 PFH got remove per frame. so you would have both systems combined in a way.
so what I would do is still save the deletedIndices and only add 1 execNextFrame and use the removing Method I implemented just in the execNextFrame and checking if a remove already happened

commy2 · 2018-07-04T05:57:31Z

@SilentSpike Can you think of a way to potential bring this issue about? Reading the code again after sleep, everything looks fine, even if the same ID is removed twice.
@jokoho48

but what I dislike is that you run over every PFH Multiple times when more than 1 PFH got remove per frame.

This almost never happens though. Removing multiple PFH in the same frame.

dedmen · 2018-07-04T07:43:25Z

but what I dislike is that you run over every PFH Multiple times when more than 1 PFH got remove per frame.

I still prefer that over the alternative of executing some code every Frame even if it's not needed 99% of the time.

jokoho48 · 2018-07-04T11:09:36Z

This almost never happens though. Removing multiple PFH in the same frame.

it does that is why I bringing this up. and also the check happens I frame After the PFH is removed

I still prefer that over the alternative of executing some code every Frame even if it's not needed 99% of the time.

then you don't read my post correctly. I sad this is a better solution but I think that it could be more optimized. in the way that we could combine both ideas and use the execNextFrame for the base removal but still save the indices that are deleted

kymckay · 2018-07-04T16:51:18Z

addons/common/fnc_removePerFrameHandler.sqf

+    [{
+        params ["_handle"];
+
+        GVAR(perFrameHandlerArray) deleteAt (GVAR(PFHhandles) select _handle);


@commy2 Yeah reading again after a sleep I don't think it would break anymore (since this line becomes deleteAt nil on any duplicated calls - which I assume won't error?).

The only exception would be, if you were to remove the same handle twice in the same frame and also addEventHandler in the next frame, would it be possible that the same index (_handle) could be refilled in between the two execNextFrame calls? Resulting in the newly added EH being deleted immediately.

This is all changed now.

commy2 · 2018-07-04T17:46:10Z

What about this? Tested it a bit with laptop, and it seems to work as expected.

kymckay · 2018-07-04T17:48:14Z

addons/common/fnc_removePerFrameHandler.sqf


-        GVAR(perFrameHandlerArray) deleteAt (GVAR(PFHhandles) select _handle);
-        GVAR(PFHhandles) set [_handle, nil];
+    if (GVAR(perFrameHandlersToRemove) isEqualTo []) then {


Missing ! ?

Ah never mind I see how it works now, because it's next frame and only runs once due to this

jokoho48 · 2018-07-04T20:25:17Z

that is the same way I have it implemented. like it

kymckay

lgtm

PabstMirror · 2018-07-23T16:21:52Z

we've rewritten this fix many times but this one is the best; no run-time performance hit

* origin/master: XEH - Compatiblity for Encore (1.84) (#953) indicate settings that need a mission restart (#894) don't skip next PFH if current one is removed while iterating through (#950) change color of name of temporarily changed settings (#952) handle input gracefully in keybinding editKey ui function (#951) Update fnc_deleteEntity.sqf (#949)

don't skip next PFH if current one is removed while iterating through

5b4b057

commy2 added the Bug Fix label Jul 3, 2018

commy2 added this to the 3.8 milestone Jul 3, 2018

dedmen approved these changes Jul 3, 2018

View reviewed changes

dedmen reviewed Jul 3, 2018

View reviewed changes

kymckay reviewed Jul 4, 2018

View reviewed changes

commy2 added 2 commits July 4, 2018 19:33

loop only once if multiple pfh are removed in the same frame

e4316d9

remove superfluous array

96e9cdb

kymckay reviewed Jul 4, 2018

View reviewed changes

kymckay approved these changes Jul 23, 2018

View reviewed changes

commy2 merged commit d6afdf6 into master Jul 23, 2018

commy2 deleted the fixSkippingPFHWhenRemoved branch July 23, 2018 10:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't skip next PFH if current one is removed while iterating through #950

don't skip next PFH if current one is removed while iterating through #950

commy2 commented Jul 3, 2018 •

edited

Loading

commy2 commented Jul 3, 2018

dedmen left a comment

dedmen Jul 3, 2018

commy2 Jul 3, 2018

dedmen Jul 3, 2018

commy2 Jul 3, 2018

kymckay commented Jul 3, 2018

jokoho48 commented Jul 3, 2018 •

edited

Loading

commy2 commented Jul 4, 2018

dedmen commented Jul 4, 2018 •

edited

Loading

jokoho48 commented Jul 4, 2018

kymckay Jul 4, 2018

kymckay Jul 4, 2018

commy2 Jul 4, 2018

commy2 commented Jul 4, 2018

kymckay Jul 4, 2018

kymckay Jul 4, 2018

jokoho48 commented Jul 4, 2018 •

edited

Loading

kymckay left a comment

PabstMirror commented Jul 23, 2018

don't skip next PFH if current one is removed while iterating through #950

don't skip next PFH if current one is removed while iterating through #950

Conversation

commy2 commented Jul 3, 2018 • edited Loading

commy2 commented Jul 3, 2018

dedmen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kymckay commented Jul 3, 2018

jokoho48 commented Jul 3, 2018 • edited Loading

commy2 commented Jul 4, 2018

dedmen commented Jul 4, 2018 • edited Loading

jokoho48 commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

commy2 commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jokoho48 commented Jul 4, 2018 • edited Loading

kymckay left a comment

Choose a reason for hiding this comment

PabstMirror commented Jul 23, 2018

commy2 commented Jul 3, 2018 •

edited

Loading

jokoho48 commented Jul 3, 2018 •

edited

Loading

dedmen commented Jul 4, 2018 •

edited

Loading

jokoho48 commented Jul 4, 2018 •

edited

Loading