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

Adds various ventcrawl QoL changes #14432

Closed
wants to merge 27 commits into from

Conversation

warior4356
Copy link
Contributor

@warior4356 warior4356 commented Sep 25, 2020

This Fixes #11813

What Does This PR Do

This PR makes a few changes to improve the experience of someone ventcrawling

  1. When you change pipenets, or if a pipenet is updated your visual pipemap will update rather than remaining static. (See gif below)
  2. You can enter any pipe if it has an open end.
  3. Leaving ventcrawl via pipes now has a 3 second delay to prevent antags from leaving by mistake.
  4. Maintenance drones have the ability to enter and exit welded vents after a 15-second interact.
  5. You can now enter passive vents.

Why It's Good For The Game

  1. Having the pipes be different than what you see while vent crawling is annoying and can result in death.
  2. Broken pipes can be quite an inconvenience to an antag.
  3. Preventing antags from accidentally leaving vent crawl into a room with a hostile or worse, via a broken pipe into an open area can easily result in death. Without change 2 the later case would leave them quite stranded.
  4. Maintenance drones should be able to move freely even in terror spider rounds. They also shouldn't have any reason to unweld a vent in a terror round.
  5. Felt like an oversight.

Images of changes

2020-09-24_23-08-10

Changelog

🆑
add: Added the ability to vent crawl into open pipes.
add: Added a 3 seconds delay to leaving ventcrawl via pipes.
add: Added the ability to enter passive vents.
add: Added the ability for maintenance drones to enter welded vents after a delay.
tweak: Ventcrawl overlay now updates when changing pipenets and when a pipenet is edited.
/:cl:

@hal9000PR
Copy link
Member

Does this mean that you can see the tubes if you go from the station loop to the security loop?

@TDSSS
Copy link
Contributor

TDSSS commented Sep 25, 2020

While the delay on leaving via broken pipes is good, it sounds like a lot when trying to just jump out of a vent to ambush someone. Can you make the timer shorter for vents than broken pipes?

@warior4356
Copy link
Contributor Author

While the delay on leaving via broken pipes is good, it sounds like a lot when trying to just jump out of a vent to ambush someone. Can you make the timer shorter for vents than broken pipes?

I can, do you have values in mind? Also, the timer is only visible to the person leaving the vent, it's not like they get a warning.

Does this mean that you can see the tubes if you go from the station loop to the security loop?

Yes. Yes it does. Changing pipe nets as you can see in the gif will re render it.

@AffectedArc07 AffectedArc07 added the Tweak This PR tweaks something ingame label Sep 25, 2020
code/ATMOSPHERICS/atmospherics.dm Outdated Show resolved Hide resolved
@BakKeener
Copy link

From what this PR offers, I am mostly up for it except for one thing.

The 3 second timer to leave a vent. There is a lot of antags who uses vents for combat purposes. This makes ambushes harder, and gives the crew a 3 second safety net knowing they can weld it without them popping out. If it was changed to give a timer for pipes, that would actually be an advantage as it would prevent people from accidentally popping out of those as they are the main culprits of tricking people out of vents.

Other than that one thing, All the other additions look great.

code/ATMOSPHERICS/atmospherics.dm Outdated Show resolved Hide resolved
code/modules/mob/living/carbon/carbon.dm Outdated Show resolved Hide resolved
code/modules/mob/living/carbon/carbon.dm Outdated Show resolved Hide resolved
code/modules/mob/living/carbon/carbon.dm Outdated Show resolved Hide resolved
Copy link
Contributor

@Kyep Kyep left a comment

Choose a reason for hiding this comment

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

I'm fine with this, but I do have one request:
make the delay on exiting a pipe onto a turf only apply if the pipe is disconnected.
IE: if you exit a pipe via an open vent, that should be instant, but if you exit a pipe that's broken there should be a delay on that (with progress bar, to prevent you accidentally leaving a broken pipe, but not delay you from intentionally exiting via a vent).

@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Sep 25, 2020
@warior4356
Copy link
Contributor Author

A few changes along with the style fixes.
All drones can bypass welded vents and the timer is 10 seconds.
The timer is removed from leaving vents, it's only on pipes.

@github-actions github-actions bot removed the Merge Conflict This PR is merge conflicted label Sep 25, 2020
@Carthusia
Copy link

Perhaps make the vent exit timer a toggle in the top right hand corner?

It starts off, but if someone wishes they can toggle it on?

@Kyep
Copy link
Contributor

Kyep commented Sep 27, 2020

Requesting you add Fixes #11813 to the PR description so that if this is merged, that issue will be closed.

@warior4356
Copy link
Contributor Author

Requesting you add Fixes #11813 to the PR description so that if this is merged, that issue will be closed.

Already fixed. That was point 1 of the PR @Kyep

@TDSSS
Copy link
Contributor

TDSSS commented Sep 27, 2020

Requesting you add Fixes #11813 to the PR description so that if this is merged, that issue will be closed.

Already fixed. That was point 1 of the PR @Kyep

No, the exact words 'Fixes #11813' need to appear in the PR's description so that github can automatically close the issue #11813 when it gets merged.

@warior4356
Copy link
Contributor Author

Sorry for the misunderstanding, the description has been updated.

@boniondev
Copy link
Contributor

I like this PR, i finally can still enter vents during terror rounds as a drone. Small curiosity though. When a drone alt clicks a welded vent, does it display a certain message in the chat or do others just see a drone vibing next to a vent?

"<span class='notice'>You climb into the ventilation system. With a resounding snap the [entrance_found] is fastened back in place.</span>")
else
visible_message("<span class='boldnotice'>[src] scrambles into the ventilation ducts!</span>", "<span class='notice'>You climb into the ventilation system.</span>")
loc = entrance_found
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
loc = entrance_found
forceMove(entrance_found)

Will make sure other logic won't break

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this. It completely breaks ventcrawling, by making all nonpipes solid black sprites.

@warior4356
Copy link
Contributor Author

@SteelSlayer I am unable to replicate your T-ray issue or the runtime. I tried to do what was in your screenshot to no avail.

Copy link
Member

@AffectedArc07 AffectedArc07 left a comment

Choose a reason for hiding this comment

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

First pass, split for reasons

user.forceMove(src.loc)
user.visible_message("You hear something squeezing through the pipes.", "You climb out the ventilation system.")
user.canmove = 0
if(do_after(user, 30, target = src))
Copy link
Member

Choose a reason for hiding this comment

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

Sources tell me target=src may be a problem and may not work well with people in vents

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Target is just for the location of the progress bar and is only visible to the user. It worked fine in testing. Is there any interaction you are concerned about?


/obj/machinery/atmospherics/AltClick(var/mob/living/L)
if(is_type_in_list(src, GLOB.ventcrawl_machinery))
if(is_type_in_list(src, GLOB.ventcrawl_machinery) || check_open())
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this is meant to be ||?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because vent crawl machinery is the list of vents, scrubbers, and passive vents. Check open on the other hand is a function that checks if the chosen atmospherics machinery has a potential connection that isn't currently filled, for example, a T junction with an open side.

TLDR: This OR lets you enter open pipes.

code/ATMOSPHERICS/components/unary_devices/vent_pump.dm Outdated Show resolved Hide resolved
if(!do_after(src, 45, target = src))
if(entrance_found)
var/datum/pipeline/P = entrance_found.returnPipenet()
if(P && (length(P.members) || P.other_atmosmch))
Copy link
Member

Choose a reason for hiding this comment

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

Doesnt this make it so you cant climb into single device pipenets? If so you need to document this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went back and checked, you are still able to climb into single device pipe nets.

Copy link
Member

@AffectedArc07 AffectedArc07 left a comment

Choose a reason for hiding this comment

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

Second batch

code/ATMOSPHERICS/atmospherics.dm Outdated Show resolved Hide resolved
code/modules/mob/living/carbon/carbon.dm Outdated Show resolved Hide resolved
code/modules/mob/living/carbon/carbon.dm Outdated Show resolved Hide resolved
code/modules/mob/living/carbon/carbon.dm Outdated Show resolved Hide resolved
code/modules/mob/living/carbon/carbon.dm Outdated Show resolved Hide resolved
else
to_chat(src, "<span class='warning'>This ventilation duct is not connected to anything!</span>")
show_ventcrawl(entrance_found)
loc = entrance_found // This can not be a forcemove. It will break the ventcrawl UI
Copy link
Member

Choose a reason for hiding this comment

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

Can you figure out why forceMove() breaks this? Force setting the loc var without other sanity checks is bad practice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can try, though this kinda falls into legacy code that I don't really understand why territory. I was just adding a comment for the sake of others trying to fix this in the future. It wasn't code I added.

code/ATMOSPHERICS/atmospherics.dm Outdated Show resolved Hide resolved
@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Jan 24, 2021
@github-actions github-actions bot removed the Merge Conflict This PR is merge conflicted label Jan 26, 2021
@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Feb 16, 2021
@github-actions github-actions bot removed the Merge Conflict This PR is merge conflicted label Feb 19, 2021
@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Mar 3, 2021
code/modules/atmospherics/machinery/atmospherics.dm Outdated Show resolved Hide resolved
user.remove_ventcrawl()
user.forceMove(target_move.loc) //handles entering and so on
user.visible_message("You hear something squeezing through the ducts.", "You climb out the ventilation system.")
if(is_type_in_list(target_move, GLOB.ventcrawl_machinery))
Copy link
Contributor

Choose a reason for hiding this comment

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

Not your fault, but yikes, this should really use a typecache instead of constantly looping through a list.

code/modules/mob/living/carbon/carbon.dm Outdated Show resolved Hide resolved
visible_message("<b>[src] scrambles into the ventilation ducts!</b>", "You climb into the ventilation system.")
src.loc = vent_found
add_ventcrawl(vent_found)
if(failed)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure failed is necessary here, since you're returning at the first chance you get.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, there used to be multiple things that caused failed.


/mob/living/proc/remove_ventcrawl()
GLOB.ventcrawlers.Remove(src)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does remove_ventcrawl get called when the mob is destroyed? If not, there might be a null value in the ventcrawlers list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically yes. Update ventcrawl gets called every time a pipenet is updated, which will remove them. I added it to destroy to be sure.

code/modules/mob/living/carbon/carbon.dm Outdated Show resolved Hide resolved
code/modules/mob/living/silicon/robot/drone/drone.dm Outdated Show resolved Hide resolved
code/modules/mob/living/silicon/robot/drone/drone.dm Outdated Show resolved Hide resolved
code/modules/mob/living/silicon/robot/drone/drone.dm Outdated Show resolved Hide resolved
code/modules/mob/living/silicon/robot/drone/drone.dm Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the Merge Conflict This PR is merge conflicted label Mar 4, 2021
@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Mar 11, 2021
@github-actions github-actions bot removed the Merge Conflict This PR is merge conflicted label Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tweak This PR tweaks something ingame
Projects
None yet
Development

Successfully merging this pull request may close these issues.

If you are a larva and is pipe walking, if you go thru a valve the pipes at the other side will be invisible.