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

Outfit improvements #19896

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

FluffyGhoster
Copy link
Contributor

Added unit test to ensure outfits do not point to abstract items.
Refactored the outfits procs a bit.
Repathed the outfits to a datum, as they should have been.
Fixed a bug where one of the outfits was randomly spawning an abstract item.

@github-actions github-actions bot added 🗺️ Mapping - Away Ship/Away Site The PR touches away ship and/or away site map files. 🗺️ Mapping - Random Ruins The PR touches random ruins map files. labels Sep 13, 2024
@DreamySkrell
Copy link
Contributor

image

strong disagree

made them objects in #18611

image

@Generalcamo
Copy link
Contributor

I'm going to agree with fluffy here. I don't see any other servers making outfits objects. It feels like there's a better way to do what you want to do, dreamy.

@FluffyGhoster
Copy link
Contributor Author

strong disagree

You're free to make objects that map to outfits, or port an outfit manager to do what you're indicating; outfits aren't an object conceptually, that we're the only ones that have them as objects (in bay derivates they're singletons, in tg derivates are datums) is a good indication of such

@FluffyGhoster
Copy link
Contributor Author

!review

@NonQueueingMatt
Copy link
Contributor

this is a pain in the ass for odyssey because it relies on object outfits

Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@DreamySkrell
Copy link
Contributor

DreamySkrell commented Sep 30, 2024

You're free to make objects that map to outfits, or port an outfit manager to do what you're indicating;

The entire point and reasoning for making them into outfits, was to allow spawning ANY arbitrary outfit (in lockers or on the ground or whatever), without having to make a separate object beforehand.

Having to make separate objects is ass, cause it's just redundant to have /datum/outfit/xyz and /obj/outfit_spawner/xyz or whatever, for every outfit. And it'd be awful to have to add these for every new outfit that anyone would want to spawn in a locker or whatever.

I see no reason to revert this. Only arguments here are:

  • "other servers have them as datums" - I don't think we should be blindly following other servers no matter what we do. As for porting concerns, if anyone ever wanted to port outfits from other servers, it'd just require changing datum to obj, all functionality being the same.
  • "they should be datums cause they're datums conceptually" - Why? Outfit objects are basically the same thing conceptually as random objects, being a thing that spawns other things. Should we also change all random objects to datums?

@FluffyGhoster
Copy link
Contributor Author

Why? Outfit objects are basically the same thing conceptually as random objects, being a thing that spawns other things. Should we also change all random objects to datums?

No they aren't, outfits have multiple objects they reference, take care of doing the equipping on the mob, registering the PDA, and a lot of other things than "it's an object on the map but a random one"

We have random objects because when they load they get replaced with an object in a list of possibile ones, which isn't ideal (probably we should make them as markers, depending on how they're used) but still vastly different than an outfit, in that they remain an object in the end, just not of the same typepath, and don't have any other functionality ontop of just replacing themselves with the final object

On the other hand, the only argument I'm seeing for this being object seems to boil down to "We should misuse the typepath system in order to save 3 more clicks to place down the actual objects the outfit is referencing when we're mapping or spawning them, that noone used once in a year or so since this was done"

To which my answer is to place down the actual objects instead; make something else to reference the outfit if you really need to, either way outfits aren't objects and shouldn't be pathed as such

Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

html/changelogs/archive/2024-03.yml Outdated Show resolved Hide resolved
@DreamySkrell
Copy link
Contributor

My opinion's unchanged. I do not think the outfit base type change is good.

Copy link
Contributor

@Geevies Geevies left a comment

Choose a reason for hiding this comment

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

code looks fine

Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link

github-actions bot commented Nov 8, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗺️ Mapping - Away Ship/Away Site The PR touches away ship and/or away site map files. 🗺️ Mapping - Random Ruins The PR touches random ruins map files. Merge Conflict Refactor Review Required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants