Skip to content
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

This fixes issue #3 for good. Should have no consequences either. #12

Merged
merged 2 commits into from
Jul 6, 2020

Conversation

R-T-B
Copy link
Collaborator

@R-T-B R-T-B commented Jul 4, 2020

This simple change should be the answer to the longstanding atmospheric bug. As it turns out, bodies orbit stars too, so why did we decide to eliminate them in this check? It literally prevented the atmospheric temp code from finding the Sun. Lol.

@R-T-B
Copy link
Collaborator Author

R-T-B commented Jul 4, 2020

I'll leave you to review this one, even though it's pretty basic. The issue is the calling function is indeed expecting a star, making this check... I don't really know, useless and harmful?

…parent star. Revert old faulty commit that edited wrong line.
@R-T-B
Copy link
Collaborator Author

R-T-B commented Jul 4, 2020

I edited the wrong line at first, hence the slight change. Still pretty easy to see what's going on here.

@R-T-B
Copy link
Collaborator Author

R-T-B commented Jul 5, 2020

This was just tested with a multistar pack where the stars orbit each other with no errors. Also, EC output is good. OhioBob has effectively said it's a good fix, so I advise merging.

@prestja prestja merged commit 84fe61c into prestja:1.9 Jul 6, 2020
@R-T-B R-T-B deleted the FixAtmoTemps branch July 6, 2020 15:24
@Keith-OHara
Copy link

Keith-OHara commented Jul 9, 2020

As it turns out, bodies orbit stars too, so why did we decide to eliminate them in this check?

The search appeared in commit ba06712 "only GetAtmoThermal for the parent star" and was intended to stop at the first star found, judging by the comment "returns the KopernicusStar parent for the given CelestialBody". So if a moon with atmosphere orbits a planet orbiting a distant star, GetBodyReferencing() would return that star, rather than the root star of the universe.
But the search stopped early, on the planet not the star. Later commits changed the comment, but it seems eventually the search will need to return a star in order to finally heat at atmosphere based on the nearest star.

The code GetBodyReferencing() doesn't do anything close to its comment anymore; it walks up to the ultimate parent, which will be the main star of the universe. I would think the original comment says what its user needs this function to do, and

 while (body?.orbit?.referenceBody != null && !body.isStar)  
   body = body.orbit.referenceBody;

would stop the loop on a star, not just before a star.

[Edited: I didn't realize at first that sun is a member of the KSP class Sun from which KopernicusStar is derived, so sun refers to this.sun and the callers of GetFluxAt() call us in turn for every star, so sun is the star of current interest.]

@R-T-B
Copy link
Collaborator Author

R-T-B commented Jul 9, 2020

The history of the function is interesting and helps explain why it functions that way. Do you feel my commit is a good way to fix the issue reported, or would you have handled it differently?

@Keith-OHara
Copy link

I can't make a strong recommendation to do the "while (!body.isStar)" because I'm not set up to compile and test this flavour of C++, but I would make the comment on GetBodyReferencing() match the behaviour you intend for this function, so you're not confused later.
@Sigma88 might find it satisfying to take a look and maybe get his original intent to work, but I can't figure out how to tag him on github.

@R-T-B
Copy link
Collaborator Author

R-T-B commented Jul 10, 2020

As far as I'd use the function, I think this is how I'd implement it. But original authors are more than welcome to weigh in. I may modify the comment to match for sure though, that should be done regardless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants