X Tutup
Skip to content

Fix pressure limit value if no atmo shield is installed#6117

Merged
sturnclaw merged 1 commit intopioneerspacesim:masterfrom
robothauler:fix_p_limit
Apr 25, 2025
Merged

Fix pressure limit value if no atmo shield is installed#6117
sturnclaw merged 1 commit intopioneerspacesim:masterfrom
robothauler:fix_p_limit

Conversation

@robothauler
Copy link
Contributor

The ship info screen shows always 0 atm if no atmospheric shield is installed.

@zonkmachine zonkmachine self-requested a review April 23, 2025 09:45
false,
{ l.ATMOSPHERIC_SHIELDING..":", atmo_shield and l.YES or l.NO },
{ l.ATMO_PRESS_LIMIT..":", string.format("%d atm", shipDef.atmosphericPressureLimit * atmo_shield_cap) },
{ l.ATMO_PRESS_LIMIT..":", string.format("%d atm", atmo_shield_cap == 0 and shipDef.atmosphericPressureLimit or shipDef.atmosphericPressureLimit * atmo_shield_cap) },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed if you change the first line to math.max(player["atmo_shield_cap"], 1).

@zonkmachine
Copy link
Member

I've tested the PR quite thoroughly and it works. The whole atmospheric shield is a bit unclear to me though. All ships in the ship market are flagged as"Atmospheric capable: Yes" and upon purchase, in the "Ship information" they all say "Atmospheric shielding: No". Are these two not describing the same thing? Are all ships supposed to be capable on landing on planets? Including DSMiner?

@fluffyfreak
Copy link
Contributor

fluffyfreak commented Apr 24, 2025

@zonkmachine Not all planets have atmospheres.
So all ships can land on planets, but if it has an atmosphere you might need atmospheric shielding

Or to put it better: They're all capable of having the shielding, but not all of them do. You could sell it because you don't intend to use it for example.

@zonkmachine
Copy link
Member

So all ships can land on planets, but if it has an atmosphere you might need atmospheric shielding

If I back some 500 commits on master (f0478a5), and start the game just to check out the ship markets, the ship models Nerodia and DSMiner only shows up on the space station on the Barnard's star start. They are the only ones that has the element atmo_shield explicitly defined and they are both set to 0.

$ grep -R atmo_shield data/ships/
data/ships/dsminer.json:		"atmo_shield": 0,
data/ships/nerodia.json:		"atmo_shield": 0,

Also, on this commit, both the ship market and the ship information page has the same term "Atmospheric shielding" and it seem to mean the same thing. If all ships are potentially ""Atmospheric capable", and always answered 'yes', then that's not very interesting info on a ship prospect.

@sturnclaw
Copy link
Member

Atmospheric Shielding on the ship information page indicates whether an atmospheric shield equipment item is installed. I do not see any mention of Atmosphere Capable on the ship market. The Atmospheric Shielding on the ship market should indicate whether an atmospheric shielding equipment item can be installed on the ship in question.

Atmospheric shielding and atmospheric pressure tolerance does need some work, but it's a better topic for after the hotfix is released I think.

There may have been a regression where the DSMiner, etc. now have the ability to receive atmospheric shielding where they did not before, and that's a topic we can tackle during the current development cycle after this hotfix is released.

@sturnclaw sturnclaw merged commit f3a1801 into pioneerspacesim:master Apr 25, 2025
@robothauler robothauler deleted the fix_p_limit branch April 25, 2025 10:38
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.

4 participants

X Tutup