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

The Great Tiny Fan Purge (attempt 2) #1973

Closed
wants to merge 3 commits into from

Conversation

PeccNeck
Copy link
Contributor

@PeccNeck PeccNeck commented Sep 9, 2024

About the PR

Replaces every single tiny fan on every POI, Bluespace event, and Shuttle with the new Directional Fan, with the sole exception of Stasis (handled by #1967).

This also fixes Vagabond's broken scrubber and vent (Discord) and a Cirno Day localization error.

A few ships also lacked fans beneath exterior airtight flaps; for consistency with other ships, these have been added.

I may have also gone overboard, as I also replaced the ones in walk-in freezers and on a number of (presumably) unused maps; please inform if I should revert or tweak any of these.

Why / Balance

Directional fans are cool. Also, I think this was the direction things were headed anyway. Please correct me if I'm wrong on any part of this.

The Vagabond and Cirno Day issues were also just flat-out bugs.

How to test

Spawn any shuttle or travel to any POI, and approach any exterior airlocks, and behold: Directional fans.

Requirements

  • I have read and I am following the Pull Request Guidelines. I understand that not doing so may get my pr closed at maintainer’s discretion
  • I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

Breaking changes

Changelog

🆑

  • tweak: All shuttles and POIs now use directional fans at airlocks
  • fix: Fixed broken pipes on NT Vagabond

@github-actions github-actions bot added Map-Shuttle Map - Shuttle Map-POI Map - POI Map-Bluespace Map - Bluespace Event Map-Admin Map - For admin use No C# FTL labels Sep 9, 2024
@arimah
Copy link
Contributor

arimah commented Sep 10, 2024

Unfortunately, by doing all ships and all POIs in the same PR, you've made it a chore and a half to review. On top of that, you've made it impossible for others to work on any of the maps in the game in parallel with this, as the map file format can't really handle concurrent edits. It would be preferable to create an issue listing all the maps in the game and creating one PR per map.

I see you've also gone and updated a bunch of ships that have been retired; these don't need to be worked on at all.

@Tych0theSynth
Copy link
Contributor

Tych0theSynth commented Sep 10, 2024

Please can you remove the following ships from this PR as I'll either do them myself as I maintain them, or because I'm actively working on them right now:

  • Lyrae
  • Bookworm
  • Ceres
  • Broadhead
  • Fishbowl
  • Bottleneck
  • Piecrust
  • Prowler
  • Hammer
  • Derelict McCargo/McHobo
  • Pathfinder

@ErhardSteinhauer
Copy link
Contributor

Also please remove the following shuttles from this PR because there is another forever PR stuck in review limbo (for the same reason your will be stuck in the same hell) where tiny fans were replaced with directional ones:

  • Brigand
  • Gasbender
  • Harbormaster
  • Kilderkin
  • Lantern
  • Legman
  • Liquidator
  • Pioneer
  • Searchlight

@dvir001
Copy link
Contributor

dvir001 commented Sep 10, 2024

This is a very bad way of doing this since we cannot lock all ships, poi and events files from other edits likes this.

This should be splited into multi PR's or just let the ships update with time

@dustylens
Copy link
Contributor

Please remove the Apothecary from your list. Much obliged.

@PeccNeck
Copy link
Contributor Author

Ah. In retrospect, yeah, bad ideas all around. I'll just close this for now and figure it out later, then. Haven't done this in a while, sorry.

I see you've also gone and updated a bunch of ships that have been retired; these don't need to be worked on at all.

Truthfully, I just went down the list of all the maps; probably should've worked smarter on that one, not harder, which I will definitely be doing next time.

@PeccNeck PeccNeck closed this Sep 10, 2024
@dvir001
Copy link
Contributor

dvir001 commented Sep 10, 2024

Ah. In retrospect, yeah, bad ideas all around. I'll just close this for now and figure it out later, then. Haven't done this in a while, sorry.

I see you've also gone and updated a bunch of ships that have been retired; these don't need to be worked on at all.

Truthfully, I just went down the list of all the maps; probably should've worked smarter on that one, not harder, which I will definitely be doing next time.

You can just take the files and split into smaller PR's and it will be ok

@tonotom1
Copy link
Contributor

I snuck additional fixes that were irking me for the Akupara into my own d-fan fix so the Akupara can be excluded too. Your hard work is still appreciated

@PeccNeck
Copy link
Contributor Author

It would be preferable to create an issue listing all the maps in the game and creating one PR per map.

probably should've worked smarter on that one, not harder, which I will definitely be doing next time.

Gone ahead and done this; issue can be found at #1980. I'm also going to begin breaking off bits and pieces as I've already done for #1975 and #1976.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FTL Map-Admin Map - For admin use Map-Bluespace Map - Bluespace Event Map-POI Map - POI Map-Shuttle Map - Shuttle No C#
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants