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

Partial Atmos Refactor #40

Conversation

stellar-novas
Copy link
Contributor

@stellar-novas stellar-novas commented Mar 23, 2024

Description

Ports space-wizards/space-station-14#22521. Depends on #39. List of changes is copied below,

  • The MolesArchived float[] field is now read-only
    • It simply gets zeroed whenever the GasMixture is set to null instead of constantly reallocating
  • Airtight query information is now cached in TileAtmosphere
    • This means that it should only iterate over anchored entities once per update.
    • Previously an invalidated atmos tile would cause ProcessRevalidate() to query airtight entities on the same tile six times by calling a combination of GridIsTileAirBlocked(), NeedsVacuumFixing(), and GridIsTileAirBlocked(). So this should help significantly reduce component lookups & entity enumeration.
    • This does change some behaviour. In particular blocked directions are now only updated if the tile was invalidated prior to the current atmos-update, and will only ever be updated once per atmos-update.
    • AFAIK this only has an effect if the invalid tile processing is deferred over multiple ticks, and I don't think it should cause any issues?
  • Fixes a potential bug, where tiles might not dispose of their excited group if their direction flags changed.
  • MapAtmosphereComponent.Mixture is now always immutable and no longer nullable
    • I'm not sure why the mixture was nullable before? AFAICT the component is meaningless if its null?
    • Space "gas" was always immutable, but there was nothing that required planet atmospheres to be immutable. Requiring that it be immutable gets rid of the constant gas mixture cloning.
    • I don't know if there was a reason for why they weren't immutable to begin with.
  • Fixes lungs removing too much air from a gas mixture, resulting in negative moles.
  • GasMixture.Moles is now [Access] restricted to the atmosphere system.
    • This is to prevent people from improperly modifying the gas mixtures (e.g., lungs), or accidentally modifying immutable mixtures.
  • Fixes an issue where non-grid atmosphere tiles would fail to update their adjacent tiles, resulting in null reference exception spam
  • Disconnected atmosphere tiles, i.e., tiles that aren't on or adjacent to a grid tile, will now get removed from the tile set. Previously the tile set would just always increase, with tiles never getting removed.
  • Removes various redundant component and tile-definition queries.
  • Removes some method events in favour of just using methods.
  • Map-exposded tiles now get updated when a map's atmosphere changes (or the grid moves across maps).
  • Adds a setmapatmos command for adding map-wide atmospheres.
  • Fixed (non-planet) map atmospheres rendering over grids.

TODO

  • Ensure bird lungs aren't broken

Changelog

🆑 ElectroJr

  • fix: Fixed a bug where partially airtight entities (e.g., thin windows or doors) could let air leak out into space

@github-actions github-actions bot added Changes: C# Changes any cs files Changes: Localization Changes any ftl files labels Mar 23, 2024
Copy link
Member

@VMSolidus VMSolidus left a comment

Choose a reason for hiding this comment

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

This isn't my full approval, I'm going to take a look at why the code is failing first, and then apply some fixes. It should not be that hard to fix, there is most likely a missing Using in WaterVaporReaction according to the test failures. Either there are changes to WaterVaporReaction.cs that we are missing, or this was an error made upstream. Regardless I should be able to fix this.

I'll submit my own approval once I've finished my in-game testing.

@DEATHB4DEFEAT DEATHB4DEFEAT added the Status: Requires This requires something else to be done before being resolved label Mar 24, 2024
@DEATHB4DEFEAT DEATHB4DEFEAT changed the title Partial atmos refactor Partial Atmos Refactor Mar 24, 2024
@DEATHB4DEFEAT DEATHB4DEFEAT added Size: 1-Very Large For especially large issues/PRs Type: Port Brings something to here from another codebase labels Mar 24, 2024
Copy link
Contributor

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

@github-actions github-actions bot added the Status: Merge Conflict FIX YOUR PR AAAGH label Mar 27, 2024
@github-actions github-actions bot removed the Status: Merge Conflict FIX YOUR PR AAAGH label Mar 27, 2024
@stellar-novas
Copy link
Contributor Author

This is partially fucked space-wizards/space-station-14#26513

Copy link
Contributor

@DangerRevolution DangerRevolution left a comment

Choose a reason for hiding this comment

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

is this something already mirrored from Delta PR's?

* Reduce atmos component queries

* Remove method events

* Cache airtight data

* Make MolesArchived nullable

* Fix airtight cache

* only get tile def once

* Immutable mixtures

* firelock queries

* misc

* misc cleanup

* Trim disconnected tiles

* Fix merge issues and bugs

* Why does the PR keep increasing in scope

* debug overlay

* Fix bugs

* Fix test, remove unused events

* Add setmapatmos command

* Fix overlays

* Add map check

* A

* Resolve conflicts with #26102

* Remove some obsolete methods
@stellar-novas
Copy link
Contributor Author

stellar-novas commented May 5, 2024

is this something already mirrored from Delta PR's?

Yes

Mnemotechnician pushed a commit to Mnemotechnician/Einstein-Engines that referenced this pull request Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes: C# Changes any cs files Changes: Localization Changes any ftl files Size: 1-Very Large For especially large issues/PRs Status: Requires This requires something else to be done before being resolved Type: Port Brings something to here from another codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gas leaking on closed windoors facing space Atmos exceptions
5 participants