Adds Parcel Wrap #34471

Centronias · 2025-01-16T22:11:52Z

About the PR

Adds Parcel Wrap, which can wrap items into nice little parcel boxes :)
(Many thanks to @lvvova1 , who put together a previous PR which I referenced quite a lot: #7974 )

Parcel wrap can be used directly on other items (AfterInteractEvent) or used via a UtilityVerb, which enables wrapping things which have storage, which'd eat the wrap rather than use it.
Wrapped parcels can be unwrapped in hand (UseInHandEvent) or via an InteractionVerb, which enables unwrapping things which are too big to pick up (currently not strictly necessary).
Wrapped parcels drop their contents on being destroyed
Wrapped parcels are recyclable, so that disposals doesn't fill up with them
Wrap and parcels are relatively extensible via yaml only changes, enabling other visuals.
I did pull over all of TG's parcel sprites, but only use about a third of them since there's a lot of handling for giftwrap visuals that I didn't deal with.

Why / Balance

More ways to hide bombs is more fun. Also I want mail to be more of a thing, and this moves in that direction.

Technical details

ParcelWrapComponent
- Makes an entity capable of wrapping things in parcel wrap
- Contains information about what kind of parcels are created
  - size
  - shape
- Specifies the sound made when wrapping things
- Has a blacklist so that you can't, eg., wrap the nuke disk
WrappedParcelComponent
- Marks an entity as being a wrapped parcel
- Has a container for the wrapped item
- Specifies the entity to create when the item's unwrapped, eg. some trash
- Specifies a sound made when unwrapping it
ParcelWrappingSystem
- Handles the logic for all of the above
- Makes WrappedParcels in such a way that they copy the size of the wrapped item, which guards against size-arbitrage.
WrappedParcel entity prototype
- Is a wrapped parcel
- Has appearance stuff to change the sprite used based on the size of the parcel

Media

https://github.com/user-attachments/assets/35df1d43-580e-4fe3-b7e7-dcfcc84d9ce8
(Shows off the doafters and examine text)

https://github.com/user-attachments/assets/f8dc8d13-f295-468c-9c17-7973a1f37e26
(Known bugs in this media: popup refers to the parcel rather than the contents; wrapping in a container can be jank when things have unusual shapes)

https://github.com/user-attachments/assets/fef7de45-ab09-47f6-a571-a8af8d3cd782
(Demos fixes to the bugs in the previous)

(Cargo order + crate)

Requirements

I have read and am following the Pull Request and Changelog Guidelines.
I have added media to this PR or it does not require an ingame showcase.

Breaking changes

All new stuff

Changelog

🆑

add: Parcel wrap, purchasable from cargo, which allows you to wrap items in paper so you can give people surprise gifts... like bombs.

github-actions · 2025-01-16T22:12:26Z

RSI Diff Bot; head commit 3ed9e5a merging into 6361a12
This PR makes changes to 1 or more RSIs. Here is a summary of all changes:

Resources/Textures/Objects/Misc/ParcelWrap/parcel_wrap.rsi

State	Old	New	Status
brown			Added
empty-roll			Added
green-with-red-stripes			Added
stripe-mask			Added
striped-base-mask			Added

Resources/Textures/Objects/Misc/ParcelWrap/parcel_wrap_trash.rsi

State	Old	New	Status
brown			Added

Resources/Textures/Objects/Misc/ParcelWrap/wrapped_parcel.rsi

State	Old	New	Status
amorphous-giftwrap-base			Added
amorphous-giftwrap-ribbon			Added
amorphous-red-with-green			Added
amorphous			Added
barcode-amorphous			Added
barcode-crate			Added
barcode-locker			Added
barcode-parcel-large			Added
barcode-parcel-medium			Added
barcode-parcel-small			Added
barcode-parcel-tiny			Added
barcode-tall-crate			Added
crate-giftwrap-base			Added
crate-giftwrap-ribbon			Added
crate-red-with-green			Added
crate			Added
humanoid-green-with-red			Added
locker-giftwrap-base			Added
locker-giftwrap-ribbon			Added
locker-red-with-green			Added
locker			Added
parcel-large-giftwrap-base			Added
parcel-large-giftwrap-ribbon			Added
parcel-large-red-with-green			Added
parcel-large			Added
parcel-medium-giftwrap-base			Added
parcel-medium-giftwrap-ribbon			Added
parcel-medium-red-with-green			Added
parcel-medium			Added
parcel-small-giftwrap-base			Added
parcel-small-giftwrap-ribbon			Added
parcel-small-red-with-green			Added
parcel-small			Added
parcel-tiny-giftwrap-base			Added
parcel-tiny-giftwrap-ribbon			Added
parcel-tiny-red-with-green			Added
parcel-tiny			Added
tall-crate-giftwrap-base			Added
tall-crate-giftwrap-ribbon			Added
tall-crate-red-with-green			Added
tall-crate			Added

Edit: diff updated after 3ed9e5a

Centronias · 2025-01-16T22:17:07Z

The RSI test failure is complaining about the licenses I've used.

TG station's repo's license is AGPL-3.0 . Does that not cover the sprites as well? What should I use instead?

K-Dynamic · 2025-01-16T22:19:47Z

Use CC license

Can you double-check the attribution of the art assets? I think the main page of the tg repo states sprites are CC or something, while GPL applies only to code.

update attribution on modified `unwrapped` sprite to better conform to CC's guidance

beck-thompson

Overall, code looks great! Lot of minor nitpick stuff. The only actual issues:
1.) Move as much of this as you can into shared (Especially the verbs)
2.) I think a doafter would make a lot of sense. They are annoying to make but it would feel a lot better!
3.) It feels weird that one wrap has an infinite amount of uses. I think making it like an item stack or something would be nice (Or at least some kind of maximum usages).

If you need help with any of this ask on discord!

Also, it would also be fun if PAIs had their name changed if they were in a parcel! Just add the component to the list in pai.yml:77 - Fully up to you though (I'm also biased 😆)

Content.Server/ParcelWrap/Components/WrappedParcelComponent.cs

Content.Server/ParcelWrap/Components/ParcelWrapComponent.cs

Content.Server/ParcelWrap/EntitySystems/ParcelWrappingSystem.cs

beck-thompson · 2025-01-18T07:08:44Z

Resources/Prototypes/Entities/Objects/Misc/ParcelWrap.yml

+        - state: parcel-medium
+          map: [ "enum.WrappedParcelVisuals.Layer" ]
+    - type: WrappedParcel
+      unwrapSound: /Audio/Effects/poster_broken.ogg


This seems odd. The noise sounds great but I'm not sure that its a good idea to reuse like this (Same with the handcuffs)! I'll ask to get confirmation.

Content.Server/ParcelWrap/EntitySystems/ParcelWrappingSystem.cs

Centronias · 2025-01-18T20:27:41Z

@beck-thompson , I kinda sidestepped a few of your organizational comments by shattering the system into three files.
I wanted to keep the ParcelWrap versus WrappedParcel organizational distinction (that's why ComponentInit and the GetVerb handlers were organized kinda weirdly).
Lemme know if this is undesireable.

beck-thompson

Splitting the system up into mutliple files like that is much cleaner good job 👍

Some more prediction issues but they shouldn't be too difficult to fix! Again, if you have any questions let me know here or discord 🫡

Content.Shared/ParcelWrap/Components/ParcelWrapComponent.cs

Content.Shared/ParcelWrap/Components/WrappedParcelComponent.cs

Content.Shared/ParcelWrap/EntitySystems/ParcelWrappingSystem.ParcelWrap.cs

Content.Shared/ParcelWrap/EntitySystems/SharedParcelWrappingSystem.cs

beck-thompson · 2025-01-21T08:02:18Z

Content.Shared/ParcelWrap/Components/ParcelWrapComponent.cs

+    /// <summary>
+    /// How many times this wrap can be used to create parcels.
+    /// </summary>
+    [DataField(required: true), AutoNetworkedField, Access(typeof(SharedParcelWrappingSystem))]


You might not need the access after you move stuff around! Also if this has a default don't bother making it required

All components should have restricted access by default, but you can move typeof(SharedParcelWrappingSystem) to the access attribute on the component, not just this field.

It seems like if the [Access] on the class is omitted, the default is unrestricted. (eg. DrinkSystem was able to modify this field directly with no RA complaints).

And, following the principle of least privilege, since only this Uses field needs to be changed by systems, the friend should be set on just this field.

Content.Shared/ParcelWrap/Components/WrappedParcelComponent.cs

Content.Shared/ParcelWrap/EntitySystems/ParcelWrappingSystem.ParcelWrap.cs

Content.Shared/ParcelWrap/EntitySystems/SharedParcelWrappingSystem.cs

Content.Shared/ParcelWrap/EntitySystems/ParcelWrappingSystem.WrappedParcel.cs

Content.Server/ParcelWrap/ParcelWrappingSystem.cs

Content.Shared/ParcelWrap/Components/ParcelWrapComponent.cs

MilonPL · 2025-01-21T09:08:16Z

Content.Shared/ParcelWrap/Components/ParcelWrapComponent.cs

+    /// <summary>
+    /// How many times this wrap can be used to create parcels.
+    /// </summary>
+    [DataField(required: true), AutoNetworkedField, Access(typeof(SharedParcelWrappingSystem))]


All components should have restricted access by default, but you can move typeof(SharedParcelWrappingSystem) to the access attribute on the component, not just this field.

Content.Shared/ParcelWrap/Components/ParcelWrapComponent.cs

Content.Shared/ParcelWrap/EntitySystems/ParcelWrappingSystem.ParcelWrap.cs

Content.Shared/ParcelWrap/EntitySystems/SharedParcelWrappingSystem.cs

MilonPL · 2025-01-21T09:13:36Z

One more thing I forgot to mention (because I hate looking over yaml), for entity prototypes you should stick to the type > abstract > parent > id > name > description > components order.

replace mostly-duped client/server with if(onserver)

Centronias · 2025-01-28T18:52:36Z

@slarticodefast indicated this implementation has some issues: https://discord.com/channels/310555209753690112/770682801607278632/1331719351283351563

Follow the replies on https://discord.com/channels/310555209753690112/770682801607278632/1333271598676971570 to see the back and forth, but I'm of the opinion that there's not really any components that can just solve this, so check out my proposed changes to generalize the behavior here away from being tied to parcels:
Centronias#1

Until I get some more feedback, I don't think there's anything more I can do with this.

beck-thompson

Good job on the PR! The prediction stuff is nasty but everything seems to work now 🫡

(Someone else with more experiencing might need to weigh in on if there is a way to make it more generalizable)

Parcel Wrap

0f29e4e

Centronias added 2 commits January 16, 2025 18:04

fix TG sprite licenses

dd2a219

update attribution on modified `unwrapped` sprite to better conform to CC's guidance

ContainerContainer test failure fix

f2f3c1c

beck-thompson self-assigned this Jan 17, 2025

beck-thompson reviewed Jan 18, 2025

View reviewed changes

Just easy changes for now.

35192de

Centronias added 3 commits January 18, 2025 12:34

Imagine building your code before pushing it for review

32ee262

Merge remote-tracking branch 'origin/master' into parcel-wrap

ceb26d1

The rest of the PR comments

71c1589

beck-thompson requested changes Jan 21, 2025

View reviewed changes

MilonPL reviewed Jan 21, 2025

View reviewed changes

PR comments

abe377d

Centronias requested review from beck-thompson and MilonPL January 21, 2025 20:56

github-actions bot added the S: Needs Review Status: Requires additional reviews before being fully accepted label Jan 21, 2025

Centronias added 2 commits January 22, 2025 10:59

more comments + cargo orderability

1a720b2

whitespace: deduplicated.

bdd29df

use limitedcharges

3ed9e5a

replace mostly-duped client/server with if(onserver)

beck-thompson approved these changes Feb 1, 2025

View reviewed changes

beck-thompson added the S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. label Feb 1, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Parcel Wrap #34471

Adds Parcel Wrap #34471

Centronias commented Jan 16, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025 •

edited

Loading

Centronias commented Jan 16, 2025

K-Dynamic commented Jan 16, 2025 •

edited

Loading

beck-thompson left a comment

beck-thompson Jan 18, 2025

Centronias commented Jan 18, 2025

beck-thompson left a comment

beck-thompson Jan 21, 2025

MilonPL Jan 21, 2025

Centronias Jan 21, 2025

MilonPL Jan 21, 2025

MilonPL commented Jan 21, 2025

Centronias commented Jan 28, 2025 •

edited

Loading

beck-thompson left a comment •

edited

Loading

Adds Parcel Wrap #34471

Are you sure you want to change the base?

Adds Parcel Wrap #34471

Conversation

Centronias commented Jan 16, 2025 • edited Loading

About the PR

Why / Balance

Technical details

Media

Requirements

Breaking changes

github-actions bot commented Jan 16, 2025 • edited Loading

Centronias commented Jan 16, 2025

K-Dynamic commented Jan 16, 2025 • edited Loading

beck-thompson left a comment

Choose a reason for hiding this comment

beck-thompson Jan 18, 2025

Choose a reason for hiding this comment

Centronias commented Jan 18, 2025

beck-thompson left a comment

Choose a reason for hiding this comment

beck-thompson Jan 21, 2025

Choose a reason for hiding this comment

MilonPL Jan 21, 2025

Choose a reason for hiding this comment

Centronias Jan 21, 2025

Choose a reason for hiding this comment

MilonPL Jan 21, 2025

Choose a reason for hiding this comment

MilonPL commented Jan 21, 2025

Centronias commented Jan 28, 2025 • edited Loading

beck-thompson left a comment • edited Loading

Choose a reason for hiding this comment

Centronias commented Jan 16, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025 •

edited

Loading

K-Dynamic commented Jan 16, 2025 •

edited

Loading

Centronias commented Jan 28, 2025 •

edited

Loading

beck-thompson left a comment •

edited

Loading