Add back isStar to GetBodyReferencing

[democat3457]

#19

[R-T-B]

This looks good to me, I was about to do this myself but you beat me to it by a good 18 hours... lol. I'll accept this merge, and build a release off of it.

[Keith-OHara]

What are you thinking here ?
This change does make GetBodyReferencing() agree with its function comment, stopping the search just before the first star, but the caller still compares the return value to each star in the system.
The caller would need to compare sun to the .orbit.referenceBody of the planet that is returned, to see if sun is the local star.
Overloading KSPs default GetBodyReferencing() seems very awkward, so probably best to simply make a localStar() function that does not stop the loop early, but goes to the first found star.

[democat3457]

I wasn't too sure upon the usage of the calculateSolarFlux function, but based on its usage, I had assumed the vessel should have been in solar orbit rather than orbiting a body that's in solar orbit, sorry if that assumption was incorrect.
Additionally, I wasn't too sure whether the GetBodyReferencing(CelestialBody) method was used elsewhere (perhaps in one of Kopernicus' dependents?), so I didn't change that.

[R-T-B]

Ugh, this is what I get for trying to rush-merge something while at work. Sorry.

"Overloading KSPs default GetBodyReferencing() seems very awkward, so probably best to simply make a localStar() function that does not stop the loop early, but goes to the first found star."

That would be the right way to do this. As this is, I have a strong feeling this may actually break atmospheric temps on planets again, I will test when I get home. I tested it via remote desktop briefly and it looked correct but I'm not even certain I was using the correct build.

Sorry for being overeager there.

[R-T-B]

Proposal:

What if we just fixed the one function that seems to need this special use case of looking for a star, the one under // Get Thermal Stats?

Like this?

if (sun == GetBodyReferencing(vessel.mainBody.referenceBody))

EDIT: A quick test indicates this works:

https://img.techpowerup.org/200720/screenshot58.jpg

I will implement it on my bleeding edge branch first, before pushing it to main this time.

[democat3457]

Wouldn't that pass null to GetBodyReferencing if the vessel is orbiting the Sun? You probably want the referenceBody of the body returned by GetBodyReferencing, like
if (sun == GetBodyReferencing(vessel.mainBody).referenceBody)
However, this would be false if the vessel was orbiting the Sun, so you'd want
if (sun == GetBodyReferencing(vessel.mainBody) || sun == GetBodyReferencing(vessel.mainBody).referenceBody)

[R-T-B]

"Wouldn't that pass null to GetBodyReferencing if the vessel is orbiting the Sun?"

Yeah, I thought about that, but thought it should be ok, it should get shutdown here:

The GetBodyReferencing code:

    public static CelestialBody GetBodyReferencing(CelestialBody body)
    {
        while (body?.orbit?.referenceBody != null && !body.orbit.referenceBody.isStar)
        {
            body = body.orbit.referenceBody;
        }

        return body;
    }

"while (body?.orbit?.referenceBody != null " for the nullcheck that should stop trouble.

Which would then return back body, AKA the star it's orbiting.

Or am I wrong somewhere? It's been a long day. I guess a quick test case would be to teleport to the sun and see?

Actually, I think you might be right, because we are calling methods on Null and that is never good. My brain is just dead, lol.

[R-T-B]

Or maybe I'm right, dunno. I just tested it in Solar orbit and no null refs to be found? Actually, it seems to be behaving perfectly.

Test on my branch if you'd like. It's in the latest release (as are your particles democat3457!)

github.com/R-T-B/Kopernicus/

[democat3457]

Actually, you are correct with the null - although

if (sun == GetBodyReferencing(vessel.mainBody).referenceBody)

still applies. With the current logic, a vessel orbiting the Mun wouldn't pass the check because the method passes Kerbin, of which the method will return Kerbin, not the Sun.

[R-T-B]

Will look into that soon, thanks. Does seem logical. There are various ways to fix that, of course. I'll push an attempt to bleeding this evening.

[Keith-OHara]

I wasn't too sure upon the usage of the calculateSolarFlux function, but based on its usage, I had assumed the vessel should have been in solar orbit rather than orbiting a body that's in solar orbit, sorry if that assumption was incorrect.

calculateSolarFlux finds the solar flux for a craft anywhere, and also sets the atmospheric temperature for the body that craft might be flying in or landed upon. The second function is the issue here.

Additionally, I wasn't too sure whether the GetBodyReferencing(CelestialBody) method was used elsewhere

It was absent in Kopernicus 1.8.1, appearing only in the panelsFix branch not any release, so there should be no-one depending on it. Having this subtly-different override of a KSP function seems unwise.

if (sun == GetBodyReferencing(vessel.mainBody.referenceBody))

Well, this is a very roundabout way of getting things right only sometimes.
If we are landed on Kerbin, this passes Kerbol to getBodyReferencing, which exits immediately because Kerbol is a star, giving back a reference to Kerbol. Thus we defeated the attempt of getBodyReferencing to find the body just before the nearest star.
If we are landed on Laythe, we pass Jool to getBodyReferencing, which finds that Jool orbits a star so returns Jool as it intended, but sun takes the values only of stars so the following code never runs. Laythe's air has no solar-dependent temperature.

Someday I will update my gcc to this newfangled C#, but it will take a while to learn.

[democat3457]

Well, this is a very roundabout way of getting things right only sometimes.
If we are landed on Kerbin, this passes Kerbol to getBodyReferencing, which exits immediately because Kerbol is a star, giving back a reference to Kerbol. Thus we defeated the attempt of getBodyReferencing to find the body just before the nearest star.
If we are landed on Laythe, we pass Jool to getBodyReferencing, which finds that Jool orbits a star so returns Jool as it intended, but sun takes the values only of stars so the following code never runs. Laythe's air has no solar-dependent temperature.

This should work, though, right? @Keith-OHara

if (sun == GetBodyReferencing(vessel.mainBody).referenceBody)

[Keith-OHara]

This should work, though, right?

if (sun == GetBodyReferencing(vessel.mainBody).referenceBody)

I think so.
But, don't you think it is confusing
to have a overloaded function getBodyReferencing() potentially different to the same-named function in KSP
that works hard to stop just short of the nearest star,
only to take one more step to get that star ?

Compared to an inline search straight to that star

CelestialBody localStar = vessel.mainBody; // initialize to search for the local star
 while (!localStar.isStar)
    localStar = localStar.referenceBody;
if (sun == localStar) 

with whatever null-pointer checks are needed and idiomatic in C#, maybe while (!localStar?.isStar && localStar?.orbit?.referenceBody != null)

[democat3457]

But, don't you think it is confusing
to have a overloaded function getBodyReferencing() potentially different to the same-named function in KSP
that works hard to stop just short of the nearest star,
only to take one more step to get that star ?

If it's used for one purpose and only once, then yes, it is confusing. However, since the method's intent is very similar to that of CelestialBody#GetBodyReferencing, the name makes sense. However however (lol), the name doesn't actually make sense since the original meaning of the name was to find the closest body referencing the second argument, or the given "star". This method, on the other hand, is aimed to find the closest star, rather than the closest body referencing the closest star.
tl;dr I agree. The inline version would be much more compact and concise, especially since the method is only used once (as far as I'm aware).

[R-T-B]

"It was absent in Kopernicus 1.8.1, appearing only in the panelsFix branch not any release, so there should be no-one depending on it. Having this subtly-different override of a KSP function seems unwise."

As mentioned in the bug report, I agree. We need a dedicated function that is not an overload.

I propose further discussion of this issue is moved to the bug report. We are splitting dialog pretty bad.