Fuel scooping rework

[Mc-Pain]

Fuel scooping was:

1. based on random numbers
2. not aware of time acceleration
3. inefficient if rate was too high (which is still unlikely to happen without time acceleration)
4. not working properly (see Fuel Scoop not functioning #5237)

Instead of this, I suggest:

1. accumulate hydrogen at rate based on speed and pressure
2. replace strict speed/pressure requirement (I think it's likely being able to scoop even at sparse atmospheres even when flying fast enough)

[impaktor]

Thanks for looking into this!

Does this PR fix the issue you link to?

Commit (structure & messages) look nice. Not sure why you would prefer (slow) division over multiplication in the last commit, though?

[Mc-Pain]

Does this PR fix the issue you link to?

Nope, since it's fixed already but there are other things I complained about

Commit (structure & messages) look nice. Not sure why you would prefer (slow) division over multiplication in the last commit, though?

I'd prefer readability over obscurity.
a / 300000 looks nicer than a * 0.00000333

How much impact slower (?) division would have in this case?
Are we sure if the compiler (gcc 13.2.1 in my case) won't optimize this?

It could replace dividing with multiplying by (1 / divisor) at compile time, and vice versa.

UPD
disassembling shows that it doesn't:

                                        const double rate = speed_times_density * double(m_stats.fuel_scoop_cap) / 300000.0;
    6b4c:       f2 0f 59 c3             mulsd  %xmm3,%xmm0
    6b50:       f2 0f 5e 05 00 00 00 00         divsd  0x0(%rip),%xmm0        # 6b58 <Ship::StaticUpdate(float) [clone .part.0]+0xa88>  6b54: R_X86_64_PC32     .LC137-0x4

I will remove this commit unless it is okay to improve readability at cost of speed

[impaktor]

I will remove this commit unless it is okay to improve readability at cost of speed

he-he, @Web-eWorks gets final say.

Also, it's not clear to me (from memory when I looked at the code a few hours ago), where that 300000 comes from, should it be a named constant? Doesn't matter much since it was that way from before. I'm just thinking loud.

[Mc-Pain]

Also, it's not clear to me (from memory when I looked at the code a few hours ago), where that 300000 comes from

it comes from d66d2d1, where 0.00001f became 0.00000333f (scooping capacity for fuel scoop has been tripled so overall rate should remain unchanged)

[sturnclaw]

A single division is not an issue unless the method in question is being run >1,000x per frame. It's in the ballpark of 4-6x slower than multiplication (for precise details consult the intel SSE ISA documentation), but that's the difference between 1-2 cycles and 8-12 cycles.

I'm much more leery about having a raw dimensionless constant of such magnitude in the source file. I'd at the very least recommend that constant be its own named variable with a comment indicating its purpose and unit, so that it can later be loaded from Lua or other gameplay-appropriate sources if needed. (For example, determining the fuel flow rate/atmospheric richness based on composition.)

[Mc-Pain]

I'm much more leery about having a raw dimensionless constant of such magnitude in the source file.

Still can be improved.
Could be additionally multiplied by hydrogen amount in atmosphere (x0.9 for Jupiter having ~90% Hydrogen)
Or, additionally, try to predict atmospheric composition based on height (more Hydrogen at upper layers)

But we still don't know where original 0.00001f appeared from. Perhaps that meant to be tons/m^3 (equal to g/cm^3)

Actual Hydrogen density at 273 K and 1 atm = 0.0000899 g/cm^3 (9 times bigger) and we already have pressure to scale it.
Not sure about temperature (melting point = 14.01 K)

[Mc-Pain]

@Web-eWorks what if I replace the last commit with one replacing 0.00000333f by constant?

// used for calculating a fuel scoop rate
const double hydrogen_pressure = 0.00000333f

[sturnclaw]

Apologies for the delay in getting back around to this PR - you've probably seen what took up my time.

what if I replace the last commit with one replacing 0.00000333f by constant?

As long as the constant has a comment indicating the unit conversion performed by employing the constant (i.e. m/s * kg/m^3 -> kg of h2) it's a good idea in my eyes. The main thing I'm concerned about here is that it's easy to follow the logical / physical process attempting to be modelled in this algorithm, so future contributors can understand and improve it if needed. Your current version is fairly easy to follow, but could perhaps be expressed in terms of minimum/total flow rate rather than using an intermediate speed*density product to determine whether scooping is happening.

Let me know when you've arrived on a model you're happy with and I'll review it for merge.

[Mc-Pain]

Now I'm happy enough, though I was not intended to rework the model completely.

Just the thing about scooping not happening when any of (speed, pressure) is below limits.
Perhaps it would be correct if we set a limit on their product instead and remove randomization of process (I am certain that H2 rate on such gas flow would not be fluctuating much)

Otherwise, imagine if such a place on Earth with sufficiently low oxygen rate appears randomly from natural causes (i.e. not including industrial pollution or closed space with a lot of humans without air vent)

[sturnclaw]

(I am certain that H2 rate on such gas flow would not be fluctuating much)

Otherwise, imagine if such a place on Earth with sufficiently low oxygen rate appears randomly from natural causes (i.e. not including industrial pollution or closed space with a lot of humans without air vent)

If I remember correctly, on Earth a disproportionate quantity of O2 is concentrated in the troposphere and other elements behave the same in large gas giants, where there are striated layers of different types/mixtures of gases rather than a 100% homogeneous atmosphere.

That might be a useful thing to model in terms of variable H2 gas flow, if I'm reading your comment correctly. However, that would be a task for another follow-up PR well in the future when we have a more complex and complete atmospheric simulation with multiple atmospheric layers (and an implementation of scanning gameplay for users to become aware of atmospheric composition).

In the context of this PR, a uniform H2 flow rate controlled only by atmospheric density is fine by me.

Finally, something to look at for this or a future PR might be controlling the scoop rate by the atmosOxidizing SystemBody parameter, as that is intended to control whether the atmosphere is primarily comprised of hydrogen/ammonia/etc. or oxygen-based compounds. The parameter is not exactly scientific, but that's partially associated with the low number of users in the codebase.

[Mc-Pain]

Can I merge this or I should wait for review then?

[sturnclaw]

Generally, unless a change is a small single-line bugfix we prefer to at least take a cursory glance at the code and test the changed mechanic to ensure it works as intended. This means that if no community members do any testing and report back their findings, testing and review usually has to wait until a developer is finished with their in-flight projects and can "switch gears" to building and reviewing the branch.

I've not yet been able to test this branch, but I can review the code at the moment - I've noticed one issue, but otherwise the code looks quite clean to me.

[sturnclaw]

Looks good to me, apologies for the merge delay.