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

Makes the station Z level into a list instead of a single define #30297

Merged
merged 17 commits into from Sep 11, 2017

Conversation

imtakingabreakdontatme
Copy link
Contributor

For multi z

I left a define for the main/first floor of multi z maps because I don't want to rewrite weather right now, and we might want some things to only impact the main floor.

I'll either add mapping helpers that add more zs to the list or beg for help to add/read lists from the map jsons

@TechnoAlchemisto
Copy link
Contributor

Really good PR Kor, keep it up! Really looking forward to more work like this in the future!

Stay frosty!

@@ -14,6 +14,8 @@ GLOBAL_LIST_INIT(diagonals, list(NORTHEAST, NORTHWEST, SOUTHEAST, SOUTHWEST))
//Go away Urist, I'm restoring this to the longer list. ~Errorage
GLOBAL_LIST_INIT(accessable_z_levels, list(1,3,4,5,6,7)) //Keep this to six maps, repeating z-levels is ok if needed

GLOBAL_LIST_INIT(station_z_levels, list(2))

Copy link
Contributor

Choose a reason for hiding this comment

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

why don't these use defines?
make these use defines.
the one above it too.

@AnturK AnturK added Tweak Refactor Makes the code harder to read and removed Tweak labels Aug 31, 2017
@Cyberboss
Copy link
Member

Trevis

@@ -519,7 +519,7 @@
if(I == src)
continue
var/mob/M = I
if(M.z in GLOB.z_levels_station && !M.stat)
if(M.z in GLOB.station_z_levels && !M.stat)
Copy link
Member

Choose a reason for hiding this comment

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

Brackets around the in statement. Precedence is stupid with it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean if((M.z in glob.station_z_levels) && !M.stat)

Also is it really important enough to warrant manually changing 130 lines if the rest of the codebase is already doing it

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it is that important, since there's a very good chance byond will go "okay, that list exists, okay, M.stat is FALSE, let's continue under this if"

@imtakingabreakdontatme
Copy link
Contributor Author

@Cyberboss

I did the thing

Copy link
Member

@Cyberboss Cyberboss left a comment

Choose a reason for hiding this comment

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

A lot more of the same, have to say I really dislike the ZLEVEL_STATION_PRIMARY defne in most cases its used

@@ -488,7 +488,7 @@
text += " <span class='boldannounce'>died</span>"
else
text += " <span class='greenannounce'>survived</span>"
if(fleecheck && ply.current.z > ZLEVEL_STATION)
if(fleecheck && ply.current.z > ZLEVEL_STATION_PRIMARY)
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right for this context. Should work on all station z levels no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the use of the define is just autoreplace to put off having to refactor so many systems in a single PR/make sure it still works in the man time

@@ -373,7 +373,7 @@ GLOBAL_LIST_INIT(blacklisted_malf_machines, typecacheof(list(
milestones[key] = TRUE
minor_announce("[key] SECONDS UNTIL DOOMSDAY DEVICE ACTIVATION!", "ERROR ER0RR $R0RRO$!R41.%%!!(%$^^__+ @#F0E4", TRUE)

/obj/machinery/doomsday_device/proc/detonate(z_level = ZLEVEL_STATION)
/obj/machinery/doomsday_device/proc/detonate(z_level = ZLEVEL_STATION_PRIMARY)
Copy link
Member

Choose a reason for hiding this comment

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

This proc need refactoring to use a list

pickedstart = spaceDebrisStartLoc(startSide, ZLEVEL_STATION)
pickedgoal = spaceDebrisFinishLoc(startSide, ZLEVEL_STATION)
pickedstart = spaceDebrisStartLoc(startSide, ZLEVEL_STATION_PRIMARY)
pickedgoal = spaceDebrisFinishLoc(startSide, ZLEVEL_STATION_PRIMARY)
Copy link
Member

Choose a reason for hiding this comment

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

Should work for all zs

@@ -9,7 +9,7 @@
var/datum/action/innate/vest_disguise_swap/vest_disguise_action = new
var/datum/action/innate/set_droppoint/set_droppoint_action = new
var/obj/machinery/abductor/console/console
z_lock = ZLEVEL_STATION
z_lock = ZLEVEL_STATION_PRIMARY
Copy link
Member

Choose a reason for hiding this comment

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

Needs refactoring

if(ZLEVEL_STATION)
F =find_safe_turf(zlevels = ZLEVEL_STATION, extended_safety_checks = TRUE)
if(ZLEVEL_STATION_PRIMARY)
F =find_safe_turf(zlevels = ZLEVEL_STATION_PRIMARY, extended_safety_checks = TRUE)
Copy link
Member

Choose a reason for hiding this comment

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

refactoring

@@ -126,7 +126,7 @@
if(target.current.stat == DEAD || !ishuman(target.current) || !target.current.ckey)
return 1
var/turf/T = get_turf(target.current)
if(T && (T.z > ZLEVEL_STATION) || (target.current.client && target.current.client.is_afk())) //If they leave the station or go afk they count as dead for this
if(T && (T.z > ZLEVEL_STATION_PRIMARY) || (target.current.client && target.current.client.is_afk())) //If they leave the station or go afk they count as dead for this
Copy link
Member

Choose a reason for hiding this comment

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

again, all station zs should be valid

@@ -5,7 +5,7 @@
anchored = TRUE
density = TRUE
var/question = "Travel back?"
var/zlevels = list(ZLEVEL_STATION)
var/zlevels = list(ZLEVEL_STATION_PRIMARY)
Copy link
Member

@Cyberboss Cyberboss Sep 5, 2017

Choose a reason for hiding this comment

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

why not use the GLOB list?

@Cyberboss
Copy link
Member

@KorPhaeron do the last one ^

@imtakingabreakdontatme
Copy link
Contributor Author

@Cyberboss I did the signpost and rev checks

@AnturK
Copy link
Member

AnturK commented Sep 10, 2017

This is just personal opinion but i feel like a function that checks this would be good even if bit more expensive so you don't have to worry about in precedence.

Copy link
Member

@AnturK AnturK left a comment

Choose a reason for hiding this comment

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

Don't see any obvious issues beside few systems that probably should work for all z's

@optimumtact optimumtact dismissed Cyberboss’s stale review September 10, 2017 21:27

cyberboss back at it again

Copy link
Member

@Cyberboss Cyberboss left a comment

Choose a reason for hiding this comment

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

Just make sure to do the refactors after this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactor Makes the code harder to read
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants