Fix EVABoundFix NRE loop on null KFSM state (#19)#20
Open
edujoliver wants to merge 1 commit into
Open
Conversation
EVABoundFix.Hook unconditionally constructs a new EVABoundFix, whose constructor dereferences eva.On_bound_fall, eva.On_bound_land and eva.st_bound_fl directly. Under some conditions (certain custom EVA suits, partially-loaded vessels, specific mod combinations) one or more of those fields are still null when Hook is called. Because FirstPersonEVA.Update() invokes Hook every frame, a single broken state produces a NullReferenceException at ~60 messages per second, flooding Player.log and degrading frame time. Validate the relevant fields before constructing EVABoundFix and skip silently if any is null. Skip reasons are deduplicated so a persistent problem logs exactly once per reason instead of every frame. Fixes FirstPersonKSP#19
Collaborator
|
@JonnyOThan Can you take a quick look at this. Looks ok to me, but I don't have a lot of time right now |
|
I don’t either but I will review this next time I’m looking at this mod. It’s definitely a problem, but I want to make sure this is the right fix. |
Collaborator
Ok. Ping me with your conclusion, if it's ok I can then do a quick build/release |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #19.
EVABoundFix.Hookunconditionally constructs a newEVABoundFixwhose private constructor dereferenceseva.On_bound_fall,eva.On_bound_landandeva.st_bound_fldirectly:Under some conditions — certain custom EVA suits, partially-loaded vessels, or specific mod combinations — one or more of those KFSM fields are still
nullwhenHookis called. The resultingNullReferenceExceptionis not caught.Because
FirstPersonEVA.Update()callsHookevery frame whenever it detects a freshKerbalEVA, a single broken state produces an NRE at ~60 messages per second, floodingPlayer.log(the report in #19 measured ~250 NREs over ~5 seconds) and degrading frame time.Fix
Validate
eva,eva.vessel, and the three KFSM fields before constructingEVABoundFix. If any is null, log the reason once and return cleanly. The mod silently skipsHookfor that particular EVA — the fields will populate later in normal cases andHookwill succeed on the next call.A small
HashSet<string>dedups the skip log so a persistent problem logs exactly once per distinct reason instead of every frame. No new allocations on the happy path.Verification
Tested locally on KSP 1.12.5 with the equivalent change applied via an external Harmony Prefix patch (TTE_Fix). The previously continuous NRE flood disappeared;
Player.lognow contains at most one line per broken state, and the per-frame time spent in exception handling is eliminated.Notes
+41 linestotal, most of which is comment + the per-field check block.Hooksucceeds (theHashSet.Addonly fires on the skip path).