Fix floating point exception in FGGasCell when gas contents approach zero#1394
Fix floating point exception in FGGasCell when gas contents approach zero#1394DheerendraTomar wants to merge 1 commit into
Conversation
andgi
left a comment
There was a problem hiding this comment.
Looks good to me.
Cheers,
Anders Gidenstam
|
I am a bit surprised by this PR:
Could you provide an example that is actually fixed by your PR ? I mean an example which crashes without your patch and works when your patch is committed. |
If a cell burst, pressure may approach zero but is at worse Patmosphere, I believe. Also, Patmosphere only reaches close to zero at the Kármán line (100km). A high altitude balloon usually goes up to 40km. The concept of a gas cell with zero volume is troubling. That would be an empty envelope, a simple piece of fabric. Basically you can put a T-shirt in the air and call it a gas cell with zero volume. @DheerendraTomar 's case seems indeed exceptional. |
d873daa to
bdb1991
Compare
|
@bcoconni @AirshipSim Thank you both for the thoughtful feedback. You're absolutely right that with the current The goal of this PR is to replace the temporary fix (464e591, which was explicitly titled "Temporary fix for issue #654") with a proper solution. Here's why the temporary clamp is problematic:
@AirshipSim You raise a good point that a zero-volume gas cell is "an empty envelope, a simple piece of fabric." That's exactly right — and that's what happens physically when a balloon bursts. Our early return handles this state cleanly rather than pretending there's always some residual gas. To reproduce the original crash (before 464e591): revert the |
The thing is, what do you actually mean by that? The term 'burst' feels like the cell is utterly destroyed in an instant. In which case, pressure, volume, etc, none of that makes any sense anymore. It sounds like a "crash event" which should be handled out-of-band. For example, the instance of the class representing the gas cell is removed from the "High Altitude Balloon" object. (Note: I have not read the buoyancy code yet, speculative)
Well, you write Pressure = AirPressure, but isn't Pressure the Pressure of the moles of lifting gas (as in Pressure * V = n(lifting gas) * RT? In which case, Mass(lifting gas) must be different from 0. An empty envelope does not equalize to ambient, it has no volume so its pressure is undefined (P = nRT / 0). This is what I tried to say with my "simple piece of fabric" image. Again, I have not read the code, so I will stop commenting on this PR. I was just trying to understand from the theoretical point of view. |
|
Thanks for the clarification, @DheerendraTomar. However the root of the problem is that we have a mechanism (the valve opening) that leads to the spontaneous creation of vacuum in the envelope. That is the physical meaning of The code in jsbsim/src/models/FGGasCell.cpp Lines 324 to 327 in 0238e96 That would be the cheapest vacuum pump in the world! Just heat up the air inside an envelope to reduce its density, open a valve and voilà! perfect vacuum has been created in the envelope. As far as I am concerned your patch does not address the actual problem and is nothing more than a glorified version of my fix, hence your PR remains a temporary fix and as such I am not willing to merge it. I will not consider the issue #654 fixed until the physics in |
|
@bcoconni Thank you — that's an excellent point about the thermodynamics, and I think you've identified the real bug. I dug into the valve equations and found something interesting. In the manual valving section, The Physically, this is wrong. As the gas cell deflates and the internal pressure drops toward ambient, the pressure differential at the valve should diminish. More importantly, once internal pressure equals ambient, air would begin flowing in through the valve to fill the void — the valve should be bi-directional. I prototyped a fix: making However, this breaks the weather balloon burst mechanism. The aircraft XML uses For the Clamping to What would you suggest as the right scope for this PR? I see two possible paths:
Happy to go either direction. |
|
@bcoconni @AirshipSim Would you please give me a direction so that I can proceed with it? |
|
@DheerendraTomar, sorry for my late response. I have missed your previous message, my apologies. Thanks for having investigated the topic in such depth. Your approach looks very interesting and is more in line with what I was expecting to address the topic properly. I will need a bit more time to digest your proposal (sorry not much free time at the moment). I should be able to give you some feedback later this week, no later than the coming week-end. In the meantime, I would like @andgi to review your suggestion as well since he is the original author of the code. |
|
Thanks for the proposal @DheerendraTomar.
Yes, that's really the point.
I would vote for option 2. Regarding the burst event, I would force the content to a small value 0.001 rather than opening the valve: diff --git a/aircraft/weather-balloon/weather-balloon.xml b/aircraft/weather-balloon/weather-balloon.xml
index 3f0557254..9e4790351 100644
--- a/aircraft/weather-balloon/weather-balloon.xml
+++ b/aircraft/weather-balloon/weather-balloon.xml
@@ -190,7 +190,6 @@
buoyant_forces/gas-cell/volume-ft3 ge buoyant_forces/gas-cell/max_volume-ft3
buoyant_forces/gas-cell/burst ge 0.5
</test>
- <output>buoyant_forces/gas-cell/valve_open</output>
</switch>
</channel>
diff --git a/scripts/weather-balloon.xml b/scripts/weather-balloon.xml
index 295f94bab..9da4ebbdc 100644
--- a/scripts/weather-balloon.xml
+++ b/scripts/weather-balloon.xml
@@ -31,6 +31,7 @@
<property>velocities/vt-fps</property>
<property>metrics/radius-ft</property>
</notify>
+ <set name="buoyant_forces/gas-cell/contents-mol" value="0.001"/>
<condition> buoyant_forces/gas-cell/burst ge 0.5 </condition>
</event>With these values, the weather balloon hits the ground at t=6752.38s instead of t=6752.8s |
6f51448 to
1f1ec9e
Compare
Replace the temporary clamp on Contents with a physically correct valve model, add explicit empty-cell handling, and decouple the balloon burst simulation from the valve. Fixes JSBSim-Team#654. Physics fixes in FGGasCell: - Scale CellHeight by GasVolume / MaxVolume so the buoyancy-driven DeltaPressure diminishes naturally as the cell deflates, letting the valve reach equilibrium. - Clamp DeltaPressure to >= 0.0 during manual valving to prevent unphysical vacuum creation when internal pressure falls to ambient. - Replace the artificial max(1E-8, ...) clamp on Contents with max(0.0, ...), and handle Contents <= 0 explicitly by setting the cell to a physically correct empty state (zero mass, ambient pressure, ballonet-only volume) and returning early. This avoids the division-by-zero FPE that the old clamp was working around. Physics fix in FGBallonet: - Replace the temporary max(1.0, ...) clamp with a clamp to the proper thermodynamic equilibrium AirPressure * MaxVolume / (R * Temperature), plus an empty-ballonet early return as a safety net. Decouple burst mechanism: - The previous weather balloon script simulated burst by opening the valve with a huge valve_coefficient, which conflicts with the new valve physics where venting correctly slows as volume drops. - Drop the valve_open output from the Burst flight control channel and have the burst event set buoyant_forces/gas-cell/contents-mol to 0.001 directly, simulating catastrophic destruction without relying on the valve model.
1f1ec9e to
58593fa
Compare
When a gas cell is rapidly emptied (e.g. weather balloon burst at high altitude), the gas Contents, Volume, and Pressure can all approach zero. This causes division-by-zero floating point exceptions in three places within FGGasCell::Calculate():
Add guards to handle these zero-denominator cases:
Fixes #654