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

atmos circulation #33380

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

atmos circulation #33380

wants to merge 7 commits into from

Conversation

Ian321
Copy link
Contributor

@Ian321 Ian321 commented Nov 18, 2024

About the PR

This PR adds air-circulation to the game. This is achieved by modifying the scrubbers and vents in a way to induce a constant exchange of air.

Why / Balance

Having air circulate is more realistic and prevents pockets of "dead" air, that have a bad temperature or mix. Because the air is constantly being exchanged, it also encourages Atmosians to set up proper recycling/waste-management and to stop just dumping it into space (also a step towards removing the gas-miners).

Tests on Oasis and Bagel lead to a constant distro kPa of 0.5 ATM without recycling and the maximum pressure if you recycle (at least N2). The 0.5 ATM is not a problem, as vents have a output multiplier of 2 (which is most likely also the reason why it's 0.5 ATM...).

Technical details

  • Added a new field "TargetPressure" to scrubbers, which is used to determine at which point all (not disabled) gasses get scrubbed.
  • Added a new field "OverflowGases" for gases that should (additionally to FilterGases) be scrubbed if the pressure is above said target.
  • Made those new fields configurable in the air alarm.
  • Adjusted the default kPa for vents to slightly above one atmosphere.
  • Removed the unused "ReplaceModePreset".

Media

a.mp4

Requirements

Breaking changes

  • None

Changelog

🆑

  • add: Air now circulates around the station.
  • add: Scrubbers now have a option for maximum pressure.
  • tweak: The gas filter inside of scrubbers is now tri-state (overflow, always, never).

@Ian321 Ian321 requested a review from Partmedia as a code owner November 18, 2024 03:11
@github-actions github-actions bot added S: Needs Review Status: Requires additional reviews before being fully accepted size/L Denotes a PR that changes 100-1000 lines. S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. Changes: UI Changes: Might require knowledge of UI design or code. labels Nov 18, 2024
@LankLTE
Copy link
Contributor

LankLTE commented Nov 18, 2024

I think this needs some pretty significant changes to almost every map, and probably atmos as a whole, before it can be added. As-is this just makes distro sabotage significantly stronger because unwrenching one vital pipe in atmos means the entire station will slowly suffocate.

@ArtisticRoomba
Copy link
Contributor

extremely, extremely cool, however my doc space-wizards/docs#319 has not been merged yet. all stations should probably be mapped with a dedicated heater/freezer thermoachine with a valve connection to distro to better assist atmosians. additionally, #33128 would help significantly. these are pretty groundbreaking changes to current atmos IMO so some care needs to be taken. like others have said, read my design doc's "drawbacks" section as to why this could be a really bad idea.

@Ian321
Copy link
Contributor Author

Ian321 commented Nov 18, 2024

@LankLTE as implemented, the scrubber only scrubs all above 1 ATM. So it won't drain the station if there is no gas to refill it. Or what pipe did you mean?

@Ian321
Copy link
Contributor Author

Ian321 commented Nov 18, 2024

DeleteAllThenGhost strikes again...
Is there no one working on fixing this fluky test?

Anyways, @ArtisticRoomba yea atmos is missing some stuff like a (pressure|temperature|smart)-(valve|pump|filter).

@LankLTE
Copy link
Contributor

LankLTE commented Nov 18, 2024

@LankLTE as implemented, the scrubber only scrubs all above 1 ATM. So it won't drain the station if there is no gas to refill it. Or what pipe did you mean?

Sorry, that is what I meant, my bad for not reading over the implementation properly (I did this on the design doc, too..)
But, speaking of design docs, I'd give this a read-over. It goes over what would be necessary to make this happen. There were some maint concerns that I don't remember exactly and now can't access but I think it's a good starting point.

@ScarKy0 ScarKy0 added P1: High Priority: Higher priority than other items, but isn't an emergency. T: New Feature Type: New feature or content, or extending existing content D2: Medium Difficulty: A good amount of codebase knowledge required. T: Balance Change Type: Balance changes through direct value changes, or changes to mechanics that affect it A: Engineering Area: Engineering department, including Atmospherics. and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Nov 18, 2024
@ArtisticRoomba
Copy link
Contributor

Can you explain this a bit more?

yea atmos is missing some stuff like a (pressure|temperature|smart)-(valve|pump|filter)

When I did testing of my design doc, I concluded that the systems are basically in place, atmos just needs to have systems mapped to support the document.

If you have concerns about filters jamming when the recyclernet is running at full blast, note that atmosians can design pressure relief systems using the pneumatic valve and other methods so the station's distronet, wastenet, and recyclernet are not clogged.

If my proposal were to be accepted, the content would have to be deployed in this order (plus suggestions):

  • All stations would have to be properly mapped with atmos flow in mind. Right now stations feature air vents and scrubbers right next to each other which is hardly effective. They should be placed far away from each other.
  • All stations would have to have either have a thermomachine array setup or a dedicated mapped area for an air cooling/heating chamber as outlined in my design doc.
  • I would wait for Gas pipe sensors #33128 to be merged so the air cooling/heating chamber can be designed better.
  • After all stations are mapped with atmos flow in mind (with the necessary infrastructure to support it), atmos flow can be introduced in this PR.
  • Gas miners would have to be tuned to produce less gas so the recyclernet's use is promoted while still allowing burn chambers to be used.

@Ian321
Copy link
Contributor Author

Ian321 commented Nov 18, 2024

@ArtisticRoomba my concern is that (without easy to use valves that open/close at a given temperature or pressure range) it would increase the skill floor of atmos by a huge amount. A person that has never played atmos before should be able to set up a stable distro.

@ArtisticRoomba
Copy link
Contributor

@ArtisticRoomba my concern is that (without easy to use valves that open/close at a given temperature or pressure range) it would increase the skill floor of atmos by a huge amount. A person that has never played atmos before should be able to set up a stable distro.

I agree, however this can actually be accomplished quite easily if someone thinks out of the box:

  • If pipenet sensors are merged, you can use the info from that to toggle a signal valve.
  • If not, you can have a small air holding chamber with an air sensor hooked up to an air alarm, and that air alarm can be hooked up to a valve.

The idea is that 90% of the system would be mapped already, atmosians would just have to set up the system for use and turn it on (filling cooling loops, ensuring values are properly set).

My proposal aims to introduce something that's actually technically challenging in real life. Right now the only thing Atmosians bother to optimize is the TEG, and even then, they all just copy off of each other and refuse to accept any suggestions for optimization.

There are tons of improvements that could be made in a whole station HVAC setup. Increasing cooling power through better working gasses in the radiator loops, experimental new setups to maximize air flow, etc. There are tons of possibilities. That is the goal.

@Ian321
Copy link
Contributor Author

Ian321 commented Nov 18, 2024

@ArtisticRoomba I have somehow missed that signal valves do already exist... Yea with the addition of #33128 this can be fully automated without a extra chamber.

@ArtisticRoomba
Copy link
Contributor

ArtisticRoomba commented Nov 18, 2024

@ArtisticRoomba I have somehow missed that signal valves do already exist... Yea with the addition of #33128 this can be fully automated without a extra chamber.

Indeed, but as per my doc, an air mixing chamber would still be desired.

I thank you for making this PR because it actually enables me to do testing of my document really really easily, setting up a test used to take an entire day. Although recognize that this should be merged following the steps I mentioned (if my proposal were to be accepted at all).

@Partmedia Partmedia added the Changes: Atmospherics Code Changes: Might require knowledge of atmospherics code & calculations. label Nov 18, 2024
@Partmedia
Copy link
Contributor

Thanks for your work on this. I'll do my best to take a look soon. Were you able to test these changes with one of the standard large station maps? It would be interesting to evaluate the performance impact of these changes, since it's possible that recirculation is going to cause a larger fraction of grid gas tiles to be excited and need processing than before.

@Partmedia
Copy link
Contributor

I know you've checked off "I have read and am following the Pull Request and Changelog Guidelines", but formatting and whitespace changes, and breaking changes e.g. 'Renamed "FilterGases" to "PriorityGases"' do not belong in the same PR as a large and potentially significant content change.

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.

Please remove all unrelated whitespace, formatting, and variable renamings that are not related to the content PR per our PR guidelines before we can review the actual content changes. Thanks.

@Ian321
Copy link
Contributor Author

Ian321 commented Nov 19, 2024

@Partmedia the white-space changes where mostly automatic (had to go in with nano just now to undo them).

The renaming from FilterGases to PriorityGases I would vote to keep, as the function of that variable has also changed.
If you where to translate this to before, you would have to add Oxygen and Nitrogen to DisabledGases, cause even if they are not in PriorityGases/FilterGases, they would still get scrubbed if the pressure was height enough.

If you insist on changing it back, I will not object.

@Partmedia
Copy link
Contributor

Thanks for reversing those automatic changes. I assumed they were automatic and felt "free" to you, but the diff is much more reviewable now that that automatic reformatting has been removed. Will be taking a closer look soon. Thanks for the help.

@Ian321
Copy link
Contributor Author

Ian321 commented Nov 19, 2024

Oh I just had an idea to optimize the code AND revert that one variable name. Instead of storing the disabled gases, it should store the overflow gases. This way we avoid looping over all the gases for each scrubber.

I will request the review once I have changed that.

@superjj18
Copy link
Contributor

I feel like a lot of hopper shifts are gonna end with station wide atmos failure lol

@LankLTE
Copy link
Contributor

LankLTE commented Nov 19, 2024

I feel like a lot of hopper shifts are gonna end with station wide atmos failure lol

how do we tell him

@ArtisticRoomba
Copy link
Contributor

Minimum effort it's just a whopping two machines to activate shift start, and maybe two loops to fill and a burn chamber activated for medium effort

The upsets to atmos will continue until player problem solving behavior improves

@Ian321
Copy link
Contributor Author

Ian321 commented Nov 19, 2024

@superjj18
a) This will only "waste" extra gas, so any pressure above 1/2 ATM inside the distro pump would be used for circulation. So nothing would change if you continue doing what you have done.
b) Here is a stable setup I just tried on Marathon. It refills a room without any problems.
Screenshot From 2024-11-19 18-21-08
c) Hopper is closing soon got closed.
image

@ArtisticRoomba
Copy link
Contributor

Note that smart atmosians can actually increase distro pressure if they ever want to increase the stations flow rate, up to the max air flow rate of all combined vents (the difference between the output pressure and standard atmospheric pressure times the number of vents)

@Ian321 Ian321 requested a review from Partmedia November 19, 2024 18:04
@SaphireLattice
Copy link
Contributor

Thanks for reversing those automatic changes. I assumed they were automatic and felt "free" to you, but the diff is much more reviewable now that that automatic reformatting has been removed. Will be taking a closer look soon. Thanks for the help.

I would like to note that GitHub and git diff allow you to ignore whitespace changes. Trailing whitespace really shouldn't be a thing in new code, and editing a file should be enough of a reason to remove them. Nevermind mixed tab/space usage, etc.

For files that weren't truly edited it is something to avoid. But throwing away fixing formatting to match standards is also not great.

@Ian321
Copy link
Contributor Author

Ian321 commented Nov 20, 2024

@Partmedia should I maybe add a configuration to maps so that mappers can opt-in/out of this? If yes, what should the default be?

@Partmedia
Copy link
Contributor

Not sure right now, I haven't gotten a chance to take a close look at the changes and try them out. Sorry for the delay and I'll do my best to look soon.

@superjj18

This comment was marked as off-topic.

@github-actions github-actions bot added the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Nov 30, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

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. Changes: Atmospherics Code Changes: Might require knowledge of atmospherics code & calculations. Changes: UI Changes: Might require knowledge of UI design or code. D2: Medium Difficulty: A good amount of codebase knowledge required. P1: High Priority: Higher priority than other items, but isn't an emergency. S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted S: Needs Review Status: Requires additional reviews before being fully accepted size/L Denotes a PR that changes 100-1000 lines. T: Balance Change Type: Balance changes through direct value changes, or changes to mechanics that affect it T: New Feature Type: New feature or content, or extending existing content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants