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

Move identity and name procs up to mob-level #3336

Merged

Conversation

out-of-phaze
Copy link
Member

Description of changes

Adds exceptions keyword argument to all overrides of GetIdCards() on /mob.
Adds return type annotation to /mob/GetIdCard().
Moves various identity-related procs up to mob-level and generalizes/cleans up the associated logic.
Removes a redundant helper proc, /proc/FindNameFromID(), that was only used in one spot. Replaces it with the almost identical proc /mob/proc/get_id_name().

Why and what will this PR improve

Cleanup and generalization of some pretty old code. Removes humantype/carbontype bias in code which should allow for carbon removal and non-humantype playable mobs in the future.

@out-of-phaze out-of-phaze added the ready for review This PR is ready for review and merge. label Sep 13, 2023
var/obj/item/card/id/id = GetIdCard()
if(istype(id))
return id.rank ? id.rank : if_no_job
else
Copy link
Contributor

Choose a reason for hiding this comment

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

Extraneous else that you seem to have trimmed off the other procs.

return FALSE
var/obj/item/clothing/mask/mask = get_equipped_item(slot_wear_mask_str)
var/obj/item/head = get_equipped_item(slot_head_str)
if(mask?.flags_inv & HIDEFACE || head?.flags_inv & HIDEFACE)
Copy link
Contributor

Choose a reason for hiding this comment

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

(mask?.flags_inv & HIDEFACE) and (head?.flags_inv & HIDEFACE)

if(mask?.flags_inv & HIDEFACE || head?.flags_inv & HIDEFACE)
return FALSE
var/decl/bodytype/our_bodytype = get_bodytype()
if(our_bodytype?.has_limbs[BP_HEAD])
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we need a should_have_limb(). Noting as a side comment that should_have_organ() needs to be moved to /mob/living too.

Copy link
Contributor

@MistakeNot4892 MistakeNot4892 left a comment

Choose a reason for hiding this comment

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

Paren addition only, other code looks fine.

@MistakeNot4892 MistakeNot4892 added awaiting author This PR is awaiting action from the author before it can be merged. and removed ready for review This PR is ready for review and merge. labels Sep 20, 2023
@out-of-phaze out-of-phaze force-pushed the feature/identity-generalization branch from 244485d to f62822f Compare October 7, 2023 15:52
@out-of-phaze out-of-phaze added ready for review This PR is ready for review and merge. and removed awaiting author This PR is awaiting action from the author before it can be merged. labels Oct 7, 2023
Copy link
Contributor

@MistakeNot4892 MistakeNot4892 left a comment

Choose a reason for hiding this comment

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

Looks good other than the conflict and CI stall.

@MistakeNot4892 MistakeNot4892 added awaiting author This PR is awaiting action from the author before it can be merged. has conflicts This PR needs updating and conflict resolution before it can be merged. and removed ready for review This PR is ready for review and merge. labels Oct 8, 2023
@out-of-phaze out-of-phaze force-pushed the feature/identity-generalization branch from f62822f to 9f651c8 Compare October 9, 2023 01:57
@out-of-phaze out-of-phaze force-pushed the feature/identity-generalization branch from 9f651c8 to 3a59897 Compare October 9, 2023 22:34
@@ -40,11 +40,14 @@
/mob/proc/show_other_examine_strings(mob/user, distance, infix, suffix, hideflags, decl/pronouns/pronouns)
return

/mob/examine(mob/user, distance, infix, suffix)
/mob/examine(mob/living/user, distance, infix, suffix)
Copy link
Contributor

@MistakeNot4892 MistakeNot4892 Oct 9, 2023

Choose a reason for hiding this comment

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

This makes the proc signature at odds with everywhere else, and ghosts will be unable to examine anyone now. Really should just gate the tail check imo.

@out-of-phaze out-of-phaze force-pushed the feature/identity-generalization branch from 3a59897 to 6e793af Compare October 10, 2023 07:05
@MistakeNot4892 MistakeNot4892 added ready for review This PR is ready for review and merge. and removed awaiting author This PR is awaiting action from the author before it can be merged. has conflicts This PR needs updating and conflict resolution before it can be merged. labels Oct 10, 2023
@MistakeNot4892 MistakeNot4892 merged commit 8821b02 into NebulaSS13:dev Oct 11, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review This PR is ready for review and merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants