search

ferram4 / Ferram-Aerospace-Research

Aerodynamics model for Kerbal Space Program
Other
238 stars 131 forks source link

Reduce garbage production by FlightGUI #173

Closed soulsource closed 7 years ago

soulsource commented 7 years ago

The current dev-build of FAR creates megabytes of garbage under certain circumstances. The reason seems to be that FAR recreates the output texts for the flight data window every physics frame, for every vessel.

While I did not invest enough time to fix the underlying issue (namely, that FixedUpdate of the GUI seems to run for vessels outside of physics range), I did spend some time to work around the huge amount of garbage being created.

Commit 8f02298 moves the creation of the display strings from FixedUpdate() to DataDisplay(), and makes sure that it's only called once per frame per active DataDisplay. This is the one commit that I'd be really glad if it would be merged, as it makes a huge performance difference for my save (which has quite a few vessels).

The other commits are more cosmetic than necessary, and I don't know if you are happy with the added StringBuilder extension methods (which I found on the net, they are under zlib license). Anyhow, those changes (very) slightly decrease garbage further, but are by far not as important as 8f02298.

ferram4 commented 7 years ago

While I did not invest enough time to fix the underlying issue (namely, that FixedUpdate of the GUI seems to run for vessels outside of physics range), I did spend some time to work around the huge amount of garbage being created.

I think that should be a quick fix then. Checking vessel.loaded or even just checking vessel == FlightGlobals.ActiveVessel in the FixedUpdate and only running if it's true should be enough to fix that underlying issue completely. Since you're aware of the undesirable behavior, do you want to try either of those fixes and see if they work and then I'll merge things?

soulsource commented 7 years ago

I've thought about this as well, but I didn't dare to change the behaviour of FlightGUI's FixedUpdate, as the data gathered there is exposed through the FAR API, and there could in principle be mods that use the API for vessels other than the currently active one, or even for unloaded ships.

soulsource commented 7 years ago

Today I've also tried to get garbage down even further, as every call to OnGUI() itself causes about 384 bytes of garbage, even if the function is empty. So, although OnGUI() in the FlightGUI returns (almost) immediately, a relatively large amount of garbage is created for savegames with many vessels/debris/asteroids. To work around this, I've tried moving the actual drawing of the GUI to a singleton (similar to the Editor GUI), and that seems to work. I haven't tested my "huge" savegame yet with this change, but adding debug output showed that now only the active vessel is really rendering the GUI, while others are just preparing data for GUI or API use.

soulsource commented 7 years ago

Regarding the FixedUpdate() function: Should I now add a check if the vessel is loaded before running it, given that some other mod might rely on some data being updated even for unloaded vessels (where most data is likely wrong anyhow, as no physics is being simulated)?

ferram4 commented 7 years ago

True, don't change the FixedUpdate function if you've separated out the GUI garbage issues. The actual data shouldn't be producing any garbage though, so leaving that should be fine. Any other changes you want to make before I merge this?

soulsource commented 7 years ago

From my point of view I'm done, and am test-playing with my local build right now. I haven't found any issues yet, so if the code is OK in your opinion, feel free to merge whenever you like.

ferram4 commented 7 years ago

@soulsource I've merged it. Question: have you made any modifications to the StringBuilder extension methods so that I can ensure that the "altered source versions must be specified" part of the lib license can be properly fulfilled?

Virindi-AC commented 7 years ago

The extension to output a floating point number seems to contain faulty logic. I wrote a better one, but it's not perfect either.

Some examples of incorrect output of Concat( this StringBuilder string_builder, float float_val, uint decimal_places, uint pad_amount, char pad_char ):

https://github.com/ferram4/Ferram-Aerospace-Research/issues/183 https://github.com/ferram4/Ferram-Aerospace-Research/pull/184

Edit: Just noticed the thing about the license. Forgot to update that, oops.