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

Remote-Promise system #1414

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

Remote-Promise system #1414

wants to merge 14 commits into from

Conversation

X39
Copy link

@X39 X39 commented Feb 5, 2021

Just noticed that i have had this "in the pipe" since 2018 ... finally creating the PR for it

When merged this pull request will:

  • Add a promise system, allowing to poll the server, calling a local method afterwards

} else {
_args call _mthd;
};
[_id, _res] remoteExec [QGVAR(requests), remoteExecutedOwner, false];
Copy link
Contributor

Choose a reason for hiding this comment

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

Last time I checked, the RE target id and the return value of REOwner do not match up in loaded local hosted multiplayer games, so this is not safe.

Copy link
Author

Choose a reason for hiding this comment

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

What do you suggest as alternative?

addons/promise/fnc_done.sqf Outdated Show resolved Hide resolved
---------------------------------------------------------------------------- */
#include "script_component.hpp"
params ["_rcv", "_mthd", "_mthdArgs", "_args", "_cb"];
private _stamp = diag_tickTime;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be noted then that this framework breaks on servers that run for 11 days.

Copy link
Author

Choose a reason for hiding this comment

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

diag_tickTime is only used to get some value that changes enough to allow reusage of the same slots and checking in isDone. As long as it changes enough, it is fine. However, i may also rework it into a simple counter that counts up. That however could introduce a race condition

@commy2
Copy link
Contributor

commy2 commented Feb 5, 2021

It bugs me that the function names are

CBA_fnc_promise_xyz

and not

cba_promise_fnc_xyz

Would prefer either PREP or camel case: CBA_fnc_promiseXyz

addons/promise/fnc_init.sqf Outdated Show resolved Hide resolved
};
};

nil
Copy link
Contributor

Choose a reason for hiding this comment

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

This function seems like bloat to me. Just run this in preInit and tell people to add requiredAddons if they really need this in preInit as well.

Copy link
Author

Choose a reason for hiding this comment

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

I am not aware of how to realize that with the macros in CfgFunctions.hpp PATHTO_FNC(promise_init);

Copy link
Contributor

Choose a reason for hiding this comment

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

Just put it in XEH preInit.

addons/promise/fnc_done.sqf Outdated Show resolved Hide resolved
addons/promise/fnc_done.sqf Outdated Show resolved Hide resolved
@commy2
Copy link
Contributor

commy2 commented Feb 5, 2021

Why does it constantly talk about "methods"? Shouldn't it be "functions" or is this specifically a thing about promises I am missing.

@X39
Copy link
Author

X39 commented Feb 5, 2021

Why does it constantly talk about "methods"? Shouldn't it be "functions" or is this specifically a thing about promises I am missing.

Can alter that. There is nothing you are missing here.

@X39
Copy link
Author

X39 commented Feb 5, 2021

It bugs me that the function names are

CBA_fnc_promise_xyz

and not

cba_promise_fnc_xyz

Would prefer either PREP or camel case: CBA_fnc_promiseXyz

Which one of the two would you prefer?

@commy2
Copy link
Contributor

commy2 commented Feb 5, 2021

Can alter that. There is nothing you are missing here.

Please do.

Which one of the two would you prefer?

Camel case, since PREP is not used much for public functions due to the advantage of the functions viewer.

Copy link
Contributor

@commy2 commy2 left a comment

Choose a reason for hiding this comment

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

Honestly, the CBA event system makes these promises either superfluous or promises should be wrappers around CBA events.

@@ -26,7 +26,7 @@ SCRIPT(findMax);

[_this] params [["_array", [], [[]]]];

if !(_array isEqualTypeAll 0) exitWith {nil};
if !(_array isEqualTypeAll 0) exitWith { nil };
Copy link
Contributor

Choose a reason for hiding this comment

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

rogue change

_args call (missionNamespace getVariable _function);
} else {
_args call _function;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This control flow is bad:

params ["_id", "_function", "_args"];
private _res = if (_function isEqualType "") then {
    _args call (missionNamespace getVariable _function);
} else {
    _args call _function;
};

Ugly. Ternaries are bad. Good Code should be linear, not branched:

params ["_id", "_function", "_args"];

if (_function isEqualType "") then {
    _function = missionNamespace getVariable [_function, {}];
};

private _res = _args call _function;

Much better.

Dunno about the implications of reading arbitrary globals as functions and calling them. Probably a lot of evil that can be done with that.

} else {
_args call _function;
};
[_id, _res] remoteExec [QGVAR(requests), remoteExecutedOwner, false];
Copy link
Contributor

Choose a reason for hiding this comment

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

remoteExec [QGVAR(requests)

Does this even work? Seems to me that requests is an array, but RE is for function names/STRINGs. I may be stupid again though.

We don't use RE in CBA or ACE, but events instead.

};
};

nil
Copy link
Contributor

@commy2 commy2 May 12, 2021

Choose a reason for hiding this comment

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

This whole function should be nuked and the default of requests should be defined either in XEH PreInit, or the create function should make this value if undefined.

The benefit of preInit is one less if-operation per function call (neglible honestly). The benefit of defining it on demand is that this makes create work in preInit for addons that do not require cba_main or this component specifically.

---------------------------------------------------------------------------- */
#include "script_component.hpp"
params ["_index", "_stamp"];
private _promise = GVAR(requests) select 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it get a promise from a list of requests?
Should it be _request or GVAR(promises) instead?

random_player, "random_function", [],
[_someLocalVariable], {
params ["_args", "_result"];
_args params ["_someLocalVariable"];
Copy link
Contributor

Choose a reason for hiding this comment

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

rogue space

params ["_args", "_result"];
_args params ["_someLocalVariable"];
diag_log _result;
diag_log _someLocalVariable;
Copy link
Contributor

Choose a reason for hiding this comment

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

If this were

systemChat str [_result];

the example would show me a result in game.

---------------------------------------------------------------------------- */
#include "script_component.hpp"
params ["_receiver", "_function", "_functionArgs", "_args", "_callback"];
private _stamp = diag_tickTime;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an absolute no go and doesn't even guarantee that there are no collisions, especially after diag_tickTime exceeded the floating point precission after 11 days. This should be a counter, possibly as string prepended with the client id (CBA_clientOwner should be a safe way to get it) to make this work in mp.

else {
GVAR(requests) set [_index, [_args, _callback, _stamp]];
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

CBA generally does not require thread safety, because writing thread safe code is a pipe dream. If a function should be atomic regardless, for example if it is supposed to be API used by the scrubs/mission makers, then we/(I?) just use this construct instead:

https://github.com/CBATeam/CBA_A3/blob/master/addons/settings/fnc_set.sqf#L27-L30

No reason to shit up the code for the scheduler.

Function that gets called when the receiver is done.
Will in the end execute the promise-code locally.

WARNING! Not intended to be called by client-code!
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure we have a Public: No doc token for this very purpose. It's also another argument to define these functions using PREP() instead of CfgFunctions, because generally only API is exposed to CfgFunctions. See the settings-component, where the CfgFunctions functions are just wrappers for the internal PREP()ed functions with some preInit handling.

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.

4 participants