-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
atmospheric temperatures are based on root star, where local star was intended #19
Comments
the purpose of example: if the system is
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 EDIT2: I can confirm that
the original intent of the code was stated in the comment a few lines above:
for a more clear example, let say we have:
|
That makes sense, given the git history.
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. |
To me, it seems like |
Do you see a separate definition for 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. |
I'm pretty certain it inherits it from KSP, and Kopernicus defines its own overload function. Also, pretty sure you meant C# :P |
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 |
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. |
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. |
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. |
Thanks for looking.
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.
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. |
Question: where is GetFluxAt()? GitHub search doesn't bring up anything. |
My mental slip when I was thinking of
|
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. |
My brain is still a little fuzzy, but I think the following could work. Critical analysis welcome, I'll be pushing it to a bleeding edge release if my tests look good: |
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): |
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. |
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. |
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. |
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. |
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. |
nice |
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
Kopernicus/src/Kopernicus/Components/KopernicusStar.cs
Line 361 in 4c39fe8
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
Kopernicus/src/Kopernicus/Components/KopernicusStar.cs
Line 361 in 0ae9b2d
The text was updated successfully, but these errors were encountered: