atmospheric temperatures are based on root star, where local star was intended

[Keith-OHara]

Using a planet pack like GEP (https://forum.kerbalspaceprogram.com/index.php?/topic/169664-181-191-grannus-expansion-pack-v121-5-july-2020/&do=findComment&comment=3823219) we see that the side of body facing the root star has the hot atmosphere.

The code finding the relevant star is here https://github.com/prestja/Kopernicus/blob/4c39fe8af7443935d32bb7d14ee95dfee3820848/src/Kopernicus/Components/KopernicusStar.cs#L361 From the code history and its use, the intent seems to be to return the local star of the given body, so while (!body.isStar && body?.orbit?.referenceBody)

The code in releases 1.9.1-4 and -5 simply walk the tree up to the root star https://github.com/prestja/Kopernicus/blob/0ae9b2dfa91de6c747d8f0a1cceef486954044cf/src/Kopernicus/Components/KopernicusStar.cs#L361

[Sigma88]

the purpose of GetBodyReferencing is to return the first star in the orbital hierarchy starting from the given celestialbody

example:

if the system is

celestialbody1 orbits celestialbody2 that orbits star1 that orbits star2

GetBodyReferencing(celestialbody1) should return star1

EDIT: correction

it seems like the code is made to return not the star but the last non star object in the hierarchy

so in the example given before GetBodyReferencing(celestialbody1) should return celestialbody2

EDIT2:

I can confirm that

The code in releases 1.9.1-4 and -5 simply walk the tree up to the root star

the original intent of the code was stated in the comment a few lines above:

Returns the 'CelestialBody' directly orbiting the parent 'KopernicusStar' for a given 'CelestialBody'.

for a more clear example, let say we have:

planet1 that orbits planet2 that orbits planet3 that orbits star1 that orbits star2 that orbits star3

GetBodyReferencing(planet1) should return planet3 because it is the planet directly orbiting the first star in the tree (star1)

[Keith-OHara]

That makes sense, given the git history. The use of GetBodyReferencing() seems to expect a star as the original comment on GetBodyReferencing promised https://github.com/prestja/Kopernicus/blob/0ae9b2dfa91de6c747d8f0a1cceef486954044cf/src/Kopernicus/Components/KopernicusStar.cs#L286 But your other code uses CelstialBodyPlus::GetBodyReferencing() in a way that seems to expect a reference to the directly-orbiting planet, apparently to handle moons orbiting planets with eccentric orbits about their star https://github.com/Sigma88/Sigma-HeatShifter/blob/aa7b2762056c36a8db222b5ed3ef344712f5248a/%5BSource%5D/SigmaHeatShifter/CelestialBodyPlus.cs#L91

I cannot find a state of the code where the assumptions of what GetBodyReferencing returns are consistent, so I suppose we have to make its users follow its current defining comment.

[democat3457]

To me, it seems like CelestialBodyPlus::GetBodyReferencing(CelestialBody, CelestialBody) is used to find the CelestialBody that directly references the second argument, which in this case, is the sun, based on the body hierarchy of the first argument. This use case is similar, if not nearly exact, to the one-argument method specified by KopernicusStar.

[Keith-OHara]

Do you see a separate definition for CelestialBodyPlus::GetBodyReferencing(CelestialBody, CelestialBody) or does it inherit from CelestialBody as defined by KSP https://www.kerbalspaceprogram.com/api/class_celestial_body.html#a771e034c5f719055ec0a044d01b1318a ?

I'm not yet set up to compile this variant of C++, so it's hard for me to trace through the name-mangling. If Kopernicus defines (as opposed to overrides) its own one-argument version of this static member function, then it can have its own name parentStar() and do what Kopernicus needs.

[democat3457]

I'm pretty certain it inherits it from KSP, and Kopernicus defines its own overload function. Also, pretty sure you meant C# :P

[R-T-B]

I think I see what's going on here. My understanding of this function when I first made this commit was limited, I believe we can do far better than what we have here. I'll attempt some changes today.

EDIT: Nevermind, we alraeady have a pull request dealing with this. I'll just accept that, thanks @democat3457

[R-T-B]

This should be fixed in release 6 thanks to democat3457's pull request. It seemed to work for me, but checking atmospheric temp situations is probably advisable. Testing welcome.

[R-T-B]

It wasn't fixed, it was worse than ever. Don't know what I tested, but I think I fixed it right. You can test the current workings on my bleeding edge branch, /R-T-B/Kopernicus

Please advise if the changes work/should be merged.

[Sigma88]

why was the code changed in the first place? was there an error coming from it?

might be easier to go back to that and work from there

[R-T-B]

why was the code changed in the first place? was there an error coming from it?

might be easier to go back to that and work from there

Atmospheric temp calc was completely broken before. There's a closed issue on it somewhere.

Anyways I'm getting reports from my bleeding branch that the latest commit works. Will merge tomorrow if all seems well.

[Keith-OHara]

why was the code changed in the first place? was there an error coming from it?

Thanks for looking. No error from the code as you left it on the panelsFix2 branch, but it never applied solar heating to atmospheres. The trouble was reported at issue #3 https://github.com/prestja/Kopernicus/issues/3 Your overload of getBodyReferencing (with a single parameter, different to stock KSP's two-parameter function) takes any planet or moon and walks up the tree to return the planet directly orbiting the nearest star, but that returned planet is compared to each star by its caller in panelsFix2::getFluxAt() so we never got a match.

might be easier to go back to that and work from there

I think so. The KSP built-in getBodyReferencing() doesn't seem to fit this need, so I was thinking of a separate function localStar() to simply walk up to the star itself.

Atmospheric temp calc was completely broken before.

No no no. The atmospheric temperature calculation was not being run. The search for the parent star was failing, seemingly due to miscommunication between functions.

[democat3457]

Question: where is GetFluxAt()? GitHub search doesn't bring up anything.

[Keith-OHara]

Question: where is GetFluxAt()? GitHub search doesn't bring up anything.

My mental slip when I was thinking of CalculateFluxAt https://github.com/prestja/Kopernicus/blob/4c39fe8af7443935d32bb7d14ee95dfee3820848/src/Kopernicus/Components/KopernicusStar.cs#L272

[R-T-B]

I think so. The KSP built-in getBodyReferencing() doesn't seem to fit this need, so I was thinking of a separate function localStar() to simply walk up to the star itself.

I agree. This is what I think we should implement in the end. Abandon the overload and make a GetLocalStar() local function to get the nearest star to the bodies orbit.

As I stated on the PR (I have requested that discussion be moved over here), I will begin work to make this function localStar() and remove this unneeded overload.

[R-T-B]

My brain is still a little fuzzy, but I think the following could work.

https://github.com/R-T-B/Kopernicus/commit/6a6d9a5f0a7ae91c0086e05a9dbdc94f47c579e3

Critical analysis welcome, I'll be pushing it to a bleeding edge release if my tests look good:

[R-T-B]

Thanks, all that told me is I'm probably not up to doing coding tonight, lol.

Still, one last stab at it,

Night all, see my final file revision here (it was a few commits so can't just link one):

https://github.com/R-T-B/Kopernicus/blob/b2514bc56e63a0755a5c1e68444701100b49cded/src/Kopernicus/Components/KopernicusStar.cs

[R-T-B]

That last one just got pushed to my bleeding edge branch as a proper binary release. It passed my brief teleport-to-moons-and-laythe tests, but could use more feedback if possible. I won't merge to stable until I get more feedback, we got burned by me doing that too many times already... heh.

[R-T-B]

So initial tests at my bleeding edge seem ok with users, I'm just waiting for feedback on someone trying a weird circumstance like a non-stock atmospheric moon of some kind, then I'll probably merge it.

A morning after a good nap and I took a look and it doesn't look insane with infinite loops or anything either, which is a good thing. Much better.

[Keith-OHara]

This looks good. I tried the release based on the resulting at https://github.com/R-T-B/Kopernicus/tree/dev191 and atmospheres on all planets and moons in GEP behave sensibly. I see the relevant commit 6c57a13 already on the 1.9 branch here. Soon after the first release containing the fix I'll try to check briefly and close this issue then, when the fix is available to regular users.

[R-T-B]

Thank you. It's been a busy week at work, so rather than cramming in a release at the end of the day and botching something again, I am waiting to push a release until tomorrow when things will hopefully let up a bit. I appreciate your feedback and indeed I already push the commits just have not built them yet.

[Keith-OHara]

release 1.9.1-6 has this fixed, as tested by checking atmospheres on a moon and planet around each star in a 2-star system.

[Sigma88]

nice