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

Fixes fire alarm closing shutters for adjacent area doors #5325

Closed
wants to merge 5 commits into from
Closed

Fixes fire alarm closing shutters for adjacent area doors #5325

wants to merge 5 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 28, 2023

About the pull request

From the code this looks intentional.... is it even a bug? #4880.

Explain why it's good for the game

having all adjacent shutters close in a given area just looks a bit silly.

shutters

Changelog

🆑
fix: fixes adjacent shutters closing after a fire alarm is pulled.
/:cl:

@github-actions github-actions bot added the Fix Fix one bug, make ten more label Dec 28, 2023
@ghost
Copy link
Author

ghost commented Dec 28, 2023

Said this on discord but, maybe instead we can have it so that all connected fire shutters to any adjacent shutter close as well. That way it doesn't look funky and closes all the nearby room shutters.

code/game/machinery/doors/firedoor.dm Outdated Show resolved Hide resolved
code/game/machinery/doors/firedoor.dm Outdated Show resolved Hide resolved
@Drulikar Drulikar marked this pull request as draft December 28, 2023 12:13
@Drulikar Drulikar linked an issue Dec 28, 2023 that may be closed by this pull request
3 tasks
@ghost ghost marked this pull request as ready for review December 28, 2023 13:04
Copy link
Member

@fira fira left a comment

Choose a reason for hiding this comment

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

probably intended for atmos so who cares

Copy link
Contributor

@Drulikar Drulikar left a comment

Choose a reason for hiding this comment

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

Works but it is likely desirable to just remove hallways fire alarms because there's just going to still be situations like this where the hallway is just a gigantic region so it having a firealarm affects things super far away - and even then it doesn't share fire doors that are shared by a different area:
image
image

Technically in this situation it ought to have closed the doors in "Port Fore Lifeboat Fuel Pump" and "Medical Upper" (and maybe others because this area spans a good distance) to be a proper fire barrier. So if you opt to not remove hallway firealarms, then maybe consider adding the old code back but have it fire alarm that entire zone (but no further than it?) but I really do not expect this to work well. Technically the old code would have handled this situation better but obvious that has its own flaws. Alternatively we break up the areas more, and we add a way for firedoors to be linked to multiple areas in sets.

Also the fire alarms themselves don't change state visually in any way (most code bases change them to a flashing red state). We do have a state for this, just this code isn't setting it. Be sure it changes the state of all firealarms in an area if its possible for multiple firealarms to exist in a single area, and be sure it returns back to whatever code green/blue/red state.
image

@Drulikar Drulikar marked this pull request as draft December 30, 2023 06:11
@ghost
Copy link
Author

ghost commented Dec 30, 2023

Works but it is likely desirable to just remove hallways fire alarms because there's just going to still be situations like this where the hallway is just a gigantic region so it having a firealarm affects things super far away - and even then it doesn't share fire doors that are shared by a different area:

image

image

Technically in this situation it ought to have closed the doors in "Port Fore Lifeboat Fuel Pump" and "Medical Upper" (and maybe others because this area spans a good distance) to be a proper fire barrier. So if you opt to not remove hallway firealarms, then maybe consider adding the old code back but have it fire alarm that entire zone (but no further than it?) but I really do not expect this to work well. Technically the old code would have handled this situation better but obvious that has its own flaws. Alternatively we break up the areas more, and we add a way for firedoors to be linked to multiple areas in sets.

Also the fire alarms themselves don't change state visually in any way (most code bases change them to a flashing red state). We do have a state for this, just this code isn't setting it. Be sure it changes the state of all firealarms in an area if its possible for multiple firealarms to exist in a single area, and be sure it returns back to whatever code green/blue/red state.

image

Sure yeah I can do that, @Huffie56, if u want to remove the hallway fire alarms and shutters. I can finish up the pr after. Feel free to commit your changes to this pr.

@Drulikar
Copy link
Contributor

Drulikar commented Dec 31, 2023

I'll note its probably fine to leave the shutters in hallways - I'm okay with there being no current way to deploy them. Its just this implementation of fire alarms in hallways isn't desirable to have them deploy so if we only have fire alarms in rooms it should work better with this implementation. But maybe maintainer will think we should find a better code solution to this.

@ghost
Copy link
Author

ghost commented Dec 31, 2023

I'll note its probably fine to leave the shutters in hallways - I'm okay with there being no current way to deploy them. Its just this implementation of fire alarms in hallways isn't desirable to have them deploy so if we only have fire alarms in rooms it should work better with this implementation. But maybe maintainer will think we should find a better code solution to this.

I think the optimal solution for hallways would have to involve some sort of mapping, either breaking up the areas, or something else, which again I don't know how to do. @Huffie56, what do you think?

@Huffie56
Copy link
Contributor

My personal opinion is that removing current check is fine.

we also need to remind that their isn't only hallway that share emergency shutters.
that's why we had the code check for adjacent area in the first place...

one of the most used example is med bay and the OR's when you pull for treatment center room you want all the emergency for OR's to go down to...

just test in medbay and if you find this ok then go....

if i am not very clear apology i am a bit tired...

@ghost
Copy link
Author

ghost commented Dec 31, 2023

I'm going to be gone for a bit @Huffie56, if u want to take this bug over. Closing the pr.

@ghost ghost closed this Dec 31, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Fix one bug, make ten more
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emergency shutters dont work
3 participants