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

/atom/proc/examine() rework. #4836

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

MistakeNot4892
Copy link
Contributor

@MistakeNot4892 MistakeNot4892 commented Feb 3, 2025

TODO

  • Split up examine()
  • Convert all examine() uses to new procs.
  • Remove examine() stub.

Description of changes

  • examine() is now examined_by().
  • examined_by() assembles string lists via three procs, in order:
    • get_examine_header()
    • get_examine_strings()
    • get_examine_hints()

Why and what will this PR improve

Allows control over ordering of examination output.
Nicer, more readable code.
Probably more performant given the removed string ops.

Authorship

Myself.

Changelog

Nothing player-facing.

@MistakeNot4892 MistakeNot4892 added the work in progress This PR is under development and shouldn't be merged. label Feb 3, 2025
@MistakeNot4892 MistakeNot4892 force-pushed the rework/examine branch 2 times, most recently from 666f6ea to 78a388c Compare February 4, 2025 05:33
Copy link
Member

@out-of-phaze out-of-phaze left a comment

Choose a reason for hiding this comment

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

i don't know how i managed to review this but i did.

@@ -8,15 +8,12 @@
var/mob/living/user_living = user
user_living.prepare_maneuver()

/obj/screen/maneuver/examine(mob/user, distance)
/obj/screen/maneuver/examined_by(mob/user, distance, infix, suffix)
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
/obj/screen/maneuver/examined_by(mob/user, distance, infix, suffix)
/obj/screen/maneuver/get_examine_strings(mob/user, distance, infix, suffix)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intentional, doesn't need the full proc, same for the other screen elem.

Copy link
Member

@out-of-phaze out-of-phaze Feb 4, 2025

Choose a reason for hiding this comment

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

issue is the return type, examined_by doesn't return strings, you need a direct to_chat

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kept as examined_by() but fixed return.

@@ -34,10 +34,10 @@
if(. && intent && parent)
parent.set_intent(intent)

/obj/screen/intent_button/examine(mob/user, distance)
/obj/screen/intent_button/examined_by(mob/user, distance, infix, suffix)
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
/obj/screen/intent_button/examined_by(mob/user, distance, infix, suffix)
/obj/screen/intent_button/get_examine_strings(mob/user, distance, infix, suffix)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per above, kept as examined_by() but fixed return.

if(Adjacent(user)) //The bag's rather thick and opaque from a distance.
to_chat(user, "<span class='info'>You peer into \the [src].</span>")
. += SPAN_INFO("You peer into \the [src].")
Copy link
Member

Choose a reason for hiding this comment

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

examined_by has a bool return type, this is invalid. you probably do legit just want to_chat() here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

code/modules/weather/_weather.dm Show resolved Hide resolved
Copy link
Member

@out-of-phaze out-of-phaze left a comment

Choose a reason for hiding this comment

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

couple more notes i found, think that's it

@@ -117,16 +117,17 @@
RETURN_TYPE(/decl/material)
return GET_DECL(/decl/material/solid/metal/steel)

/obj/machinery/door/firedoor/examine(mob/user, distance)
/obj/machinery/door/firedoor/get_examine_strings(mob/user, distance, infix, suffix)
Copy link
Member

Choose a reason for hiding this comment

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

missed a to_chat on line 142

for(var/path in uncreated_component_parts)
var/obj/item/thing = path
to_chat(user, "<span class='notice'> [initial(thing.name)] ([uncreated_component_parts[path] || 1])</span>")
. += SPAN_NOTICE(" [initial(thing.name)] ([uncreated_component_parts[path] || 1])")
if(length(.) && show_directly)
Copy link
Member

Choose a reason for hiding this comment

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

not a fan of this pattern, i think maybe display_parts should always return a list and stuff that passes show_directly = TRUE should just use to_chat() directly, or there should be a separate proc that does that

@@ -50,10 +50,10 @@
return .
return ..()

/obj/item/sticky_pad/examine(mob/user)
/obj/item/sticky_pad/examined_by(mob/user, distance, infix, suffix)
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
/obj/item/sticky_pad/examined_by(mob/user, distance, infix, suffix)
/obj/item/sticky_pad/get_examine_strings(mob/user, distance, infix, suffix)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Split into examine_strings and examine_hints.

/obj/item/duct_tape/examine()
return stuck ? stuck.examine(arglist(args)) : ..()
/obj/item/duct_tape/examined_by(mob/user, distance, infix, suffix)
return stuck ? stuck.examined_by(arglist(args)) : ..()
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
return stuck ? stuck.examined_by(arglist(args)) : ..()
return stuck ? stuck.examined_by(user, distance, infix, suffix) : ..()

i don't really like the use of arglist here tbh. like, we've standardized the args, haven't we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I replaced it everywhere I saw it, missed this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work in progress This PR is under development and shouldn't be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants