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

Fix gas pipe leaking when unanchoring or breaking them #33774

Merged
merged 3 commits into from
Dec 8, 2024

Conversation

slarticodefast
Copy link
Member

@slarticodefast slarticodefast commented Dec 8, 2024

About the PR

Currently when unanchoring a pipe some of the gas will leak from it, but not all of it. This means you can pick it up as a small 200L tank in your backpack and release the gas into the atmosphere by anchoring and unachoring it a second time, which can be potentially abused.
This PR solves this by making the leak amount equal to $V_{pipe} / V_{total}$ which was suggested by @Partmedia in #29574.
The removed pipe will then always be completely empty.

Why / Balance

Fixes #32632

Technical details

I simplified the way the leak amount is calculated, removing the pressure dependency and making it only volume dependend, which makes more sense. The warning popup will now show up when the pipes pressure is above standard atmospheric pressure, below that the gas still leaks, but the amount is insignificant.

When the pipe is unanchored, the node group update will be scheduled for the next tick. However, this will mean we cannot yet remove the gas from the pipe element since it is still connected with its neighbors. We cannot do this in the next tick either, because the information about the pipe entity and its node is lost by then. To solve this we force a manual update for the scheduled node groups. This should be fine performance-wise since it only happens once when a player unanchors it and the update won't be repeated in the next tick.

Media

pipe.leak.mp4

Requirements

Breaking changes

none

Changelog
🆑

  • fix: Fixed pipes still containing gas after unanchoring them.

@github-actions github-actions bot added S: Needs Review Status: Requires additional reviews before being fully accepted size/M Denotes a PR that changes 100-999 lines. S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Dec 8, 2024
Copy link
Contributor

@Partmedia Partmedia left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. Thanks especially for looking into past related work and giving important technical context. I have a few questions inline.

@beck-thompson beck-thompson added T: Bugfix Type: Bugs and/or bugfixes P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. D2: Medium Difficulty: A good amount of codebase knowledge required. S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. A: Engineering Area: Engineering department, including Atmospherics. A: General Interactions Area: General in-game interactions that don't relate to another area. and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Dec 8, 2024
Copy link
Contributor

@Partmedia Partmedia left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. Up to you what you want to do with the pressure warning. Please go ahead and merge when you're finished.

@slarticodefast
Copy link
Member Author

I reverted the pressure warning popup change as discussed above.

@slarticodefast slarticodefast merged commit ede8a1d into space-wizards:master Dec 8, 2024
12 checks passed
@slarticodefast slarticodefast removed the S: Needs Review Status: Requires additional reviews before being fully accepted label Dec 8, 2024
@slarticodefast slarticodefast deleted the pipeleak branch December 8, 2024 23:20
Doctor-Cpu pushed a commit to Doctor-Cpu/space-station-14 that referenced this pull request Jan 24, 2025
…#33774)

* fix gas pipe unanchoring

* remove unneeded update

* revert popup change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: Engineering Area: Engineering department, including Atmospherics. A: General Interactions Area: General in-game interactions that don't relate to another area. D2: Medium Difficulty: A good amount of codebase knowledge required. P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. size/M Denotes a PR that changes 100-999 lines. T: Bugfix Type: Bugs and/or bugfixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pipe unanchoring is exploitable
3 participants