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

Common - Fix CBA_missionTime in long missions (above 7 days) #1324

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

Conversation

commy2
Copy link
Contributor

@commy2 commy2 commented Apr 7, 2020

When merged this pull request will:

I have no tested this yet, so there may be typos. It should all be as simple as this though, unless I am missing something.

@Vdauphin
Copy link
Contributor

Vdauphin commented Apr 7, 2020

Definitely this PR simplify the CBA_missionTime update and resolve CBA_missionTime being stuck due to tiny add of value to a big number.

But, I feel it missing the opportunity to give support to do addition/subtraction/comparison of two big number without lost of precision or unintended results. I prefer the vector representation of the time for precision (math operation give us the necessary to deal with) and the classic number representation for backward compatibility. Also this representation give the possibility for mod to choose the time representation they need.

@PabstMirror PabstMirror modified the milestones: 3.15.1, 3.15.2 Apr 18, 2020
Copy link
Contributor

@Vdauphin Vdauphin left a comment

Choose a reason for hiding this comment

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

Looks good otherwise
Looks like the CBA_missionTime is accurate around +-1s when time is above 1e7 (115 days)

@commy2
Copy link
Contributor Author

commy2 commented May 31, 2020

@Vdauphin

        _elapsedTime = my_oldTimeString call CBA_fnc_missionTimeDelta;

        _deltaTime = [my_oldTimeString, my_newTimeString] call CBA_fnc_missionTimeDelta;

        _missionTime = [] call CBA_fnc_missionTimeDelta;

@Vdauphin
Copy link
Contributor

Nice!
Do you plan to migrate CBA_fnc_waitAndExecute and PFH to this?

@commy2
Copy link
Contributor Author

commy2 commented May 31, 2020

WAE and WUAE sure, but PFH uses tickTime, not missionTime (does not suspend or accelerate in SP, starts from game start, not mission start, is not synchronized).

call FUNC(missionTimePFH);

// Update CBA_missionTime when synchronized, time has started, and game is not paused.
if ([GVAR(missionTimeSynchronized), time > 0, isGamePaused] isEqualTo [true, true, false]) then {
Copy link
Contributor

@mharis001 mharis001 May 31, 2020

Choose a reason for hiding this comment

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

I think this is better written as:

Suggested change
if ([GVAR(missionTimeSynchronized), time > 0, isGamePaused] isEqualTo [true, true, false]) then {
if (GVAR(missionTimeSynchronized) && time > 0 && !isGamePaused) then {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't that slower though? That shit runs every frame.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is slightly faster.

Result:
0.0028 ms

Cycles:
10000/10000

Code:
[cba_common_missionTimeSynchronized, time > 0, isGamePaused] isEqualTo [true, true, false]
Result:
0.0021 ms

Cycles:
10000/10000

Code:
cba_common_missionTimeSynchronized && {time > 0 && {!isGamePaused}}

Copy link
Contributor Author

@commy2 commy2 May 31, 2020

Choose a reason for hiding this comment

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

What was cba_common_missionTimeSynchronized in your example? It will be true all mission long.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was true; the overall result for both statements was also true.

Copy link
Contributor

Choose a reason for hiding this comment

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

hows the speed without the lazy eval?
creating array, calling "true" command 3 times, creating another array, calling isEqualTo command surely is a bit expensive.
But lazy eval is also expensive but in this case as commy said, we don't expect neither missionTimeSynchronized or the time check to ever be false, so why lazy eval?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, no lazy eval is the fastest.

Result:
0.0017 ms

Cycles:
10000/10000

Code:
cba_common_missionTimeSynchronized && time > 0 && !isGamePaused

Updated the suggestion.

@PabstMirror PabstMirror modified the milestones: 3.15.2, 3.16 Sep 16, 2020
@PabstMirror PabstMirror modified the milestones: 3.16, Ongoing Feb 19, 2021
@jonpas jonpas changed the title fix CBA_missionTime in long missions (above 7 days) Common - Fix CBA_missionTime in long missions (above 7 days) Jul 23, 2021
@jonpas
Copy link
Member

jonpas commented Jul 23, 2021

Status?

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

Successfully merging this pull request may close these issues.

6 participants