[1.1.3] BackgroundProcessing

[Merill]

Snacks doesn't appear to be using Background Processing the mod, although it does look like they're trying to do processing in the background for snacks.

My fork use background processing. I change the code. It's not really snacks anymore.

For the meal-based, it's just a if() in the update function (vs constant update that use a delta time).

I post the link to the code just in case you want to see how I deal/kill eva kerbals / use background processing with kerbals in vessels.

Edit: Ouuups, i forgot to push my code on the repo (or link the right branch).

It's here now.

https://github.com/supermerill/Snacks/blob/backgroundProcessing/Snacks/Snacks/SnackConsumer.cs

Edited May 29, 2015 by Merill
my bad

[FlowerChild]

also there's a config file which allows setting which factors affect solar panel output and a fix for locked tanks draining (So BSTM's changed version is now out of date and unnecessary).

That's good news James! So far I've just been using a recompiled version of the last one I rigged up for 0.9, but that's only because I've yet to get to the point where solar panels are unlocked in the mod.

Fuel cells and ISRUs still won't work in the background, and I'm not certain what to do about them. Neither will resource extraction stuff.

I don't envy that task man, as I could see it getting messy real quick. I'm actually ditching both of them for BTSM, as well as solar panel heat affecting energy output.

[FlowerChild]

Hey James, I'm finally getting to integrating the newest version of BP with my slow process of porting BTSM over to KSP 1.0, and looking over some of the code, I'm getting a little worried about the performance impact of some of the newer stuff.

In particular, with SolarPanel.cs I'm a bit concerned over the number of conditions that have been added to what is essentially a tight inner loop. Like, with all the Addon.Debug() statements, each of those involves a condition testing if the message is above the current debug level set in the config file. Now, with that one, hopefully branch prediction will minimize the impact of it, but it still seems rather excessive to include that kind of thing in such a performance sensitive portion of the code.

Also, with stuff like this:

				if (!Addon.config.solarOrientationMatters) { orientationFactor = 1; Addon.Debug("Orientation disabled in config file", DebugLevel.ALL); }
				if (!Addon.config.solarTemperatureMatters) { tempFactor = 1; Addon.Debug("Temperature disabled in config file", DebugLevel.ALL); }

It worries me that regardless of the settings, the full calculations for orientation and temperature are performed first, and only afterwards are those values overridden depending on the options. In other words, even if the computed values aren't used, they're still being calculated, when whether they should be calculated or not could just as easily be tested beforehand.

Obviously, speed of execution is a big concern with something like BP, and solar panels are probably the biggest potential issue there.

[jamespicone]

Hey James, I'm finally getting to integrating the newest version of BP with my slow process of porting BTSM over to KSP 1.0, and looking over some of the code, I'm getting a little worried about the performance impact of some of the newer stuff.

In particular, with SolarPanel.cs I'm a bit concerned over the number of conditions that have been added to what is essentially a tight inner loop. Like, with all the Addon.Debug() statements, each of those involves a condition testing if the message is above the current debug level set in the config file. Now, with that one, hopefully branch prediction will minimize the impact of it, but it still seems rather excessive to include that kind of thing in such a performance sensitive portion of the code.

Also, with stuff like this:

                if (!Addon.config.solarOrientationMatters) { orientationFactor = 1; Addon.Debug("Orientation disabled in config file", DebugLevel.ALL); }
                if (!Addon.config.solarTemperatureMatters) { tempFactor = 1; Addon.Debug("Temperature disabled in config file", DebugLevel.ALL); }

It worries me that regardless of the settings, the full calculations for orientation and temperature are performed first, and only afterwards are those values overridden depending on the options. In other words, even if the computed values aren't used, they're still being calculated, when whether they should be calculated or not could just as easily be tested beforehand.

Obviously, speed of execution is a big concern with something like BP, and solar panels are probably the biggest potential issue there.

Premature optimisation is the root of all evil. This is all constant-factor stuff, barely worth caring about from a performance perspective, tbh. I'll move the orientationFactor stuff, because that at least takes two-digit amounts of floating point. The temperature factor stuff is almost unnoticeable - it's a single floatcurve.evaluate().

I feel pretty confident noting that all of this should have next to no noticeable performance impact.

[FlowerChild]

Premature optimisation is the root of all evil.

While I agree, I will also note that not addressing such low hanging fruit that has no impact on code readability or maintainability is probably a close second in the evil department

I also have no idea what you're planning in terms of expanding on the code in question that would make it "premature".

I feel pretty confident noting that all of this should have next to no noticeable performance impact.

Perhaps, but I think what was more concerning to me was the overall trend there. There was really no solid reason that I could discern for the stuff I mentioned.

Edited June 13, 2015 by FlowerChild

[jamespicone]

but I think what was more concerning to me was the overall trend there. There was really no solid reason that I could discern for the stuff I mentioned.

Debugging info is pretty important, not dumping debug info at incredible rates into everybody's logs is also important (and actually firing the logging info is much slower than a /single boolean test that almost always fails/). The 'inside-out' version where I have different versions of the same function depending on the logging level is worse for maintainability and readability.

I consider the no-orientation and no-temperature paths to be 'not the main line', and as a result the code ended up being written so that they were spurs off the main path instead of testing for orientation/temperature relevance before doing the calculations. I honestly don't think the performance impact here is relevant. This code isn't cycle-counting levels of performance critical.

[FlowerChild]

This code isn't cycle-counting levels of performance critical.

Fair enough man. When a code segment can easily be run a couple of thousand times a second though, I just tend to wince when I see unnecessary conditionals thrown into the mix

[almagnus1]

Have you done any testing with either TAC-LS or USI Kolonization System?

RE: Debug stuff, you might want to take a look at this article, and consider wrapping it in a while where the compiler will remove the code during an optimization pass:

C# “Debug only†code that should run only when “turned onâ€Â

http://stackoverflow.com/questions/5080477/c-sharp-debug-only-code-that-should-run-only-when-turned-on

It's the same basic idea s the C++ #ifdef __DEBUG__ preprocessor flag, just .NET-ified.

[jamespicone]

Have you done any testing with either TAC-LS or USI Kolonization System?

RE: Debug stuff, you might want to take a look at this article, and consider wrapping it in a while where the compiler will remove the code during an optimization pass:

C# “Debug only†code that should run only when “turned onâ€Â

http://stackoverflow.com/questions/5080477/c-sharp-debug-only-code-that-should-run-only-when-turned-on

It's the same basic idea s the C++ #ifdef __DEBUG__ preprocessor flag, just .NET-ified.

I haven't tested against either of those mods. Some people above apparently have, and I don't think they were having any problems.

I don't want to remove the debug code in release versions. It's turned on or off by a configuration file, with the point being that if someone is having a problem I can have them turn on logging info and then tell me what is output.

[Gryphon]

I haven't tested against either of those mods. Some people above apparently have, and I don't think they were having any problems.

I don't want to remove the debug code in release versions. It's turned on or off by a configuration file, with the point being that if someone is having a problem I can have them turn on logging info and then tell me what is output.

Yup. There's a difference between debug code that only needs to exist for the dev, and debug code that needs to run in the production code, but only when you need it. Clearly, KSP mods have to do a LOT of that. It's a WILD mod ecosystem out there.

[DocMop]

Is this 1.05 compatible?

[panzerwaffe044]

Does anyone still use it? Does it work with 1.0.5?

Edited February 14, 2016 by panzerwaffe044

[jamespicone]

I haven't tested against versions of KSP more recent than 1.0.2, but I've been told that it functions with 1.0.5. It probably won't blow up your save, but maybe backup just in case.

[ShotgunNinja]

Hey James, I'm developing a mod that use your awesome BackgroundProcessing and have found some problems in the code, that went on and fixed in my version:

• off-rail vessels position must be converted to ScaledSpace (maybe this changed recently...)
  • fix: in solar panel HandleResource(), replace any occurence of 'v.GetWorldPos3D()' with 'ScaledSpace.LocalToScaledSpace(v.GetWorldPos3D()) '
• PhysicsGlobal.SolarLuminosity is 0 (zero) before loading first vessel in a game session
  • fix: hard-code solar luminosity value (3.1609409786213E+24)
• load-only-most-recent-version machinery doesn't work if the MonoBehaviour class names are the same
  • fix: rename the MonoBehaviour class 'BackgroundProcessing' to 'BackgroundProcessing_MAJOR_MINOR_REVISION_BUILD'

In any case, I want to take the chance to thank you for your work. Best regards!

Edited February 27, 2016 by ShotgunNinja

[KvaNTy]

1 hour ago, ShotgunNinja said:

• PhysicsGlobal.SolarLuminosity is 0 (zero) before loading first vessel in a game session
  • fix: hard-code solar luminosity value (3.1609409786213E+24)

Solar luminosity can change at some day. You forget about tons of planetary/scaling/rebalance mods that can alter such things.
 

1 hour ago, ShotgunNinja said:

• off-rail vessels position must be converted to ScaledSpace (maybe this changed recently...)
  • fix: in solar panel HandleResource(), replace any occurence of 'v.GetWorldPos3D()' with 'ScaledSpace.LocalToScaledSpace(v.GetWorldPos3D()) '

Can you explain why?
I use my own fork of BackgroundProcessing, and that's why interested in enhancing it.

[ShotgunNinja]

@KvaNTy

I understand hard-coding the solar luminosity is not ideal, but right now you notice that there is no EC generation after starting/loading a game session (because PhysicsGlobal.SolarLuminosity is set to 0). Only after a vessel (any vessel) became active that constant is set by the game engine. Maybe a compromise may be to use the hard-coded SolarLuminosity only if the game engine is returning 0 for it. In this way there is only the minor inconvenience that the value is wrong (but still some EC is generated) only if these two conditions are met:

• no vessel was ever active from the start of the game session
• another planetary/scaling/rebalance mod is used

I think it is a good compromise.

 

For your second question, honestly I'm not sure myself. But you can verify this by orbiting a vessel with a solar panel and printing the EC amount in the log. You'll notice that the sun raycasting isn't really working unless you convert the vessel position to scaled space.

[jamespicone]

2 hours ago, ShotgunNinja said:

Hey James, I'm developing a mod that use your awesome BackgroundProcessing and have found some problems in the code, that went on and fixed in my version:

• off-rail vessels position must be converted to ScaledSpace (maybe this changed recently...)
  • fix: in solar panel HandleResource(), replace any occurence of 'v.GetWorldPos3D()' with 'ScaledSpace.LocalToScaledSpace(v.GetWorldPos3D()) '
• PhysicsGlobal.SolarLuminosity is 0 (zero) before loading first vessel in a game session
  • fix: hard-code solar luminosity value (3.1609409786213E+24)
• load-only-most-recent-version machinery doesn't work if the MonoBehaviour class names are the same
  • fix: rename the MonoBehaviour class 'BackgroundProcessing' to 'BackgroundProcessing_MAJOR_MINOR_REVISION_BUILD'

In any case, I want to take the chance to thank you for your work. Best regards!

ScaledSpaces: can you explain that one? I haven't seen any issues in prior releases, but I haven't looked into later-than-1.0.2.

SolarLuminosity initialisation: My in-dev code has a fix in the works.

Single-instance behaviour: Noted, will fix.

[jamespicone]

12 minutes ago, ShotgunNinja said:

Maybe a compromise may be to use the hard-coded SolarLuminosity only if the game engine is returning 0 for it.

That's my intended fix.

[ShotgunNinja]

1 minute ago, jamespicone said:

That's my intended fix.

Great! By the way I've done some painfull extensive testing in 1.0.5 and except for the previous things I reported this is a pretty solid piece of work.

[KvaNTy]

Btw, @jamespicone, I had a question for you.
What is the purpose of enumerating all part's modules(in the middle of VesselData.GetVesselData()) and just spaming logs with "Expected: ModuleName..." and "Found: AnotherModuleName..."?

[jamespicone]

6 minutes ago, KvaNTy said:

Btw, @jamespicone, I had a question for you.
What is the purpose of enumerating all part's modules(in the middle of VesselData.GetVesselData()) and just spaming logs with "Expected: ModuleName..." and "Found: AnotherModuleName..."?

The code is attempting to match up modules in ProtoPartSnapshots with the associated module in the prefab part. We need both of them to do background processing properly, and there's no easier way to figure out which one of them is which than walking both lists finding which ones match up.

Those debug messages should only be printed if the lists are out of sync - they should be giving you indexes. That'll happen if the prefabs PartModules don't match the ProtoPartSnapshot's PartModules, which is usually caused by things adding PartModules at runtime. The messages are there so that if someone has an error caused by PartModules being added at runtime I'll be able to figure it out.

The messages should be printed at WARNING level; you can turn them off in the config file.

[KvaNTy]

I suspected something like that. I just thinks its doing useless actions most of the time. No matter whether you do want to see those log or you don't.
Maybe it would be better to check if LogLevel.Warning first, and if so let it iterate ship's modules? Cos with size of the ship and number of installed mods the total number of modules grows almost exponentially.

[jamespicone]

2 hours ago, KvaNTy said:

I suspected something like that. I just thinks its doing useless actions most of the time. No matter whether you do want to see those log or you don't.
Maybe it would be better to check if LogLevel.Warning first, and if so let it iterate ship's modules? Cos with size of the ship and number of installed mods the total number of modules grows almost exponentially.

We have to iterate through the modules regardless of the log level.

Edited February 27, 2016 by jamespicone

[Svm420]

On 2/27/2016 at 6:38 AM, ShotgunNinja said:

Great! By the way I've done some painfull extensive testing in 1.0.5 and except for the previous things I reported this is a pretty solid piece of work.

Would you mind sharing your improved dll?

[ShotgunNinja]

On 3/3/2016 at 6:43 PM, Svm420 said:

Would you mind sharing your improved dll?

Sure. This is my interim version of BackgroundProcessing with some fixes for KSP 1.0.5. All credits goes to the original author, @jamespicone. Please report any problems you may found.

BackgroundProcessing for KSP 1.0.5

EDIT [March 07 2016]: updated, now do ad-hoc raycasting of celestial bodies, instead of using KSP game engine

EDIT [July 26 2016]: obsolete link removed

Edited July 26, 2016 by ShotgunNinja
obsolete link removed