Atmospheric flight fix

[starling13]

Fix the error in calculation of drag force basis components.

Leads to reduction of the overall drag on significant angles of attack, as can be seen from the first look.

[impaktor]

Cool.

FYI: "fix" commits aught to be squashed into the correct commit, as we (usually) don't want to track bug fixes in the PR itself.

Looks like there are some clang format issues still. (see wiki dev section on how to run clang-format script)

@fluffyfreak looks like appveyor is working again? I hadn't noticed until now.

[starling13]

Ok, I should read the dev wiki parts. Can somebody explain, how to
squash commits into one? Or I must create new branch with correct commit sequence?

[impaktor]

https://pioneerwiki.com/wiki/Using_git_and_GitHub#Fixing.2FUpdating_your_pull_request

If you need further help, come by IRC (you need to register a nick with nickserv to speak)

[The-EG]

Does this address #4847 ?

[starling13]

Does this address #4847 ?

No, It only fix calculation of the linear drag, not angular.
The change was discussed at the bottom part of #4556 conversation.

[WKFO]

Wait, this does NOT square the local velocity and take its x, y, z components, right? Shouldn't we separate x, y, z first and square each axis individually?

[starling13]

Wait, this does NOT square the local velocity and take its x, y, z components, right? Shouldn't we separate x, y, z first and square each axis individually?

But the operator *, overloaded for vector3d simply multiplies each component of the facteurs.
So after

const vector3d lv2(localVel * localVel);

lv2 contains squares of localVel coordinates.

[WKFO]

Oh, that's okay then. I was thinking in scalar values in my head, dumb me....

[sturnclaw]

I'll have to double-check both the original calculation and this one - the way it was originally done, the drag calculation was also responsible for about half of the lift (up to a certain point), which might be the reason the current one doesn't look entirely correct. I'll review this when I get some time over the next few days (I promise!), but if this calculation brings us closer to correct behavior I think it'll be good for merge.

[starling13]

I also found a possible bug with Planet::GetAtmosphericState.
The atmosphere temperature rises with altitude.
It is caused by the not so intuitive negative "g" value, computed in Init method:

// surface gravity = -G*M/planet radius^2
m_surfaceGravity_g = -G * sbody->GetMass() / (sbody->GetRadius() * sbody->GetRadius());

which gives positive laps rate in GetAtmospheric state:

const double lapseRate_L = -m_surfaceGravity_g / specificHeatCp; // negative deg/m

I propose to make "g" value positive and make a few corrections of sign in Planet class.
After I'v corrected this error I can see a funny heating while descending from the Earth orbit at 7 km/s.

Is it worth to open an issue and pull request, or I can place the commits in this branch, because I think of computing Mach number, using the atmosphere temperature, which will affect Atmospheric drag?

[impaktor]

Is it worth to open an issue and pull request, or I can place the commits in this branch

If it's a bug in master related to atmosphere-stuff/similar code, I'm fine with adding commits to this PR.

[The-EG]

I also found a possible bug with Planet::GetAtmosphericState.
The atmosphere temperature rises with altitude.

#4216 and some prior discussion about it in #4219

edit: I'm not informed enough to have an opinion either way, I just thought it helpful to have the prior stuff.

[starling13]

#4216 and some prior discussion about it in #4219

What was the result? The long discussion, the consensus about that laps rate troposhere model, being used, takes positive g and LR is negative itself, big diff at pull request, which cleans up the code and insulted author deleted the branch? Or language barrier prevented me from uderstanding.

[The-EG]

#4216 and some prior discussion about it in #4219

What was the result? The long discussion, the consensus about that laps rate troposhere model, being used, takes positive g and LR is negative itself, big diff at pull request, which cleans up the code and insulted author deleted the branch? Or language barrier prevented me from uderstanding.

I wasn't involved with it back then, but my take on it now is that there was reluctance to just flip the signs and call it good. I wouldn't read too much into everything else, including the branch being deleted. At least, that's not why I was pointing out that PR, only for the discussion about the formulas used.

I think if you have a solution and are willing to back it up and discuss it then it would be a good contribution to make.

[impaktor]

In accordance with the "Chesterton's fence"-analogy[*], I'm fine with changes as
long as the author knows why they're doing it (beyond "seems to work"), and
understands the original code. This was what was not properly motivated in the
previous PR, linked above, which is why it ended up on the cutting-room floor.
At least that was my involvement in the issue.

If you guys understand the code, go for it (I haven't looked at it myself).

[*] From G.K. Chesterton, The Thing (1929):

The more modern type of reformer goes gaily up to [a fence] and says, “I
don’t see the use of this; let us clear it away.” To which the more
intelligent type of reformer will do well to answer: “If you don’t see the
use of it, I certainly won’t let you clear it away. Go away and think. Then,
when you can come back and tell me that you do see the use of it, I may
allow you to destroy it.

[starling13]

The results of the correction for venus:

P.S. It seems public wiki at bitbucket.org repositories uses temporary image links...

[Zireael07]

@starling13: Nothing shows?

[starling13]

The final test results, after correction of next error with the signs of "g" and "lapse rate". Both errors make the results incorrect, but realistic.
Corrected density decrease more smooth, and temperature drops with altitude realistically untill tropopause.
Venus:



Earth:



Mars:

[starling13]

@Web-eWorks, if you found correction of drag force suitable to merging, decide if it's worth to make also atmospheric parameters fix, discussed above before merging.

[sturnclaw]

Left a few comments and requests for change - I'd appreciate the atmospheric density changes being added to this PR as (I presume) they're small enough to not be a burden while reviewing!

[starling13]

Left a few comments and requests for change - I'd appreciate the atmospheric density changes being added to this PR as (I presume) they're small enough to not be a burden while reviewing!

I made dedicated clang-format commit on Planet.cpp file because it leaves the main atmosphere computations correction commit clear and atomic.

[sturnclaw]

Left a few comments and requests for change - I'd appreciate the atmospheric density changes being added to this PR as (I presume) they're small enough to not be a burden while reviewing!

I made dedicated clang-format commit on Planet.cpp file because it leaves the main atmosphere computations correction commit clear and atomic.

That's the way we prefer things, thank you! Left one change request, but then I think this PR will be good for merge!