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

Inaccurate atmospheric temperature calculations #3

Closed
prestja opened this issue Jun 25, 2020 · 8 comments
Closed

Inaccurate atmospheric temperature calculations #3

prestja opened this issue Jun 25, 2020 · 8 comments

Comments

@prestja
Copy link
Owner

prestja commented Jun 25, 2020

https://forum.kerbalspaceprogram.com/index.php?/topic/194936-191-kopernicus-continued/&do=findComment&comment=3809504

@Keith-OHara
Copy link

The panelsFix2 branch at src/Kopernicus/Components/KopernicusStar.cs recomputes GetAtmoThermalStats for some reason, in the new code from commit 07230f3 "CalculateFlux => CalculateFluxAt"

@R-T-B
Copy link
Collaborator

R-T-B commented Jul 1, 2020

Good catch. I'm sure that was done as part of the multistar support for solar panels, but atmospheres really don't need their thermal stats recomputed in this situation, do they?

EDIT: That code would be broken in multistar situations anyways, as it only uses one "Sun" vector. I recomend removing lines 283-291 from the present master of KopernicusStar, and testing. They are commented under the heading "// Get Thermal Stats." This should allow the stock game to calculate flux properly.

We will still only get atmospheric flux calculations from one star, but honestly, that's not a huge deal right now as long as it's the closest one, and it should be in most instances (isn't that what stock does?). I hope I'm right anyways, Kopernicus is pretty foreign to me.

I'll look into it in detail tomorrow. I don't recommend acting on this until I have had time to test.

EDIT EDIT: At the end of a long night, I see no reason for that entire 07230f3 commit. I recommend removing it's changes, pending testing of course.

@R-T-B
Copy link
Collaborator

R-T-B commented Jul 1, 2020

Don't revert that afterall, it's needed for multistar support.

I think I fixed it, but I need a reliable way to test this issue. Is there a test scenario I can setup easily for it?

@R-T-B
Copy link
Collaborator

R-T-B commented Jul 1, 2020

Tested and fixed in PR #7

@R-T-B
Copy link
Collaborator

R-T-B commented Jul 2, 2020

Or so I thought, it's still not right. It appears to be applying temps to the vessel component and not the general atmosphere, as confirmed by AeroGUI. Still working on this.

@Keith-OHara
Copy link

Shucks. Must be something else in PanelsFix2.
The timeline could be confusing: this issue was actually opened a day before the merge (PanelsFix2 6022e3b) that brought the into this branch.
Release 1.9.1-1 has the air temperature cold at the poles, warm at the equator, and 1.9.1-2 has uniform air temperature,

@R-T-B R-T-B mentioned this issue Jul 3, 2020
@R-T-B
Copy link
Collaborator

R-T-B commented Jul 4, 2020

Unfortunately my proposed PR is not good enough. EC is not functioning correctly at all.

I'm working on fixing that. For now, do not merge.

prestja added a commit that referenced this issue Jul 6, 2020
This fixes issue #3 for good.  Should have no consequences either.
@R-T-B
Copy link
Collaborator

R-T-B commented Jul 6, 2020

This was fixed with last merge and will be included in next release, so closing.

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 a pull request may close this issue.

3 participants