Skip to content

Fix #56: Fix CommnetNetwork.Update performance#58

Merged
gotmachine merged 3 commits into
KSPModdingLibs:devfrom
JonnyOThan:fix-commnet-speed
Jul 14, 2022
Merged

Fix #56: Fix CommnetNetwork.Update performance#58
gotmachine merged 3 commits into
KSPModdingLibs:devfrom
JonnyOThan:fix-commnet-speed

Conversation

@JonnyOThan
Copy link
Copy Markdown
Contributor

The throttling logic that was supposed to update it once every 5 seconds was broken

The throttling logic that was supposed to update it once every 5 seconds was broken
@gotmachine
Copy link
Copy Markdown
Contributor

For reference, the original logic is :

if (!queueRebuild && !commNet.IsDirty)
{
    double currentTime = Time.timeSinceLevelLoad;

    double activeVesselInterval = 
        (FlightGlobals.ActiveVessel == null || FlightGlobals.ActiveVessel.packed) 
            ? packedInterval 
            : unpackedInterval;

    if (prevUpdate + activeVesselInterval > currentTime)
    {
        double focusedVesselInterval = focusedVessel == null ? 0.0 : packedInterval;
        if (prevUpdate + focusedVesselInterval > currentTime)
        {
            graphDirty = true;
            return;
        }
    }
}

You patch is entirely bypassing the first two checks (if (!queueRebuild && !commNet.IsDirty)), that doesn't seem right to me.

I think the rest kinda works, although rate-limiting based on real-time instead of in-game time is quite questionable, but I guess that's beyond the point.

@JonnyOThan
Copy link
Copy Markdown
Contributor Author

JonnyOThan commented Jul 13, 2022

I can add those checks back in, but those two fields are never set to true in the stock code.

Yeah, I'm guessing the packed vs unpacked distinction is meant for timewarp, but shrug.

@JonnyOThan
Copy link
Copy Markdown
Contributor Author

Hey, not sure if new commits automatically ping or not. I added in the checks.

@gotmachine
Copy link
Copy Markdown
Contributor

I will make the patch disabled by default.

Even if the throttling mechanism is originally intended, this patch introduce a breaking change from CommNet usually being in sync with the game loop to it using potentially vastly outdated data.

0.5 RL seconds updates at 100000x warp means essentially garbage results, and while this doesn't matter too much in stock, there are mods relying on CommNet for various stuff.

@gotmachine gotmachine merged commit 3f79be4 into KSPModdingLibs:dev Jul 14, 2022
@JonnyOThan
Copy link
Copy Markdown
Contributor Author

Thanks! But note the stock code ALSO can do 0.5 second intervals, if focusedVessel is not null.

@JonnyOThan JonnyOThan deleted the fix-commnet-speed branch July 18, 2022 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants