Skip to content

Fix clientside storage Whitelists#24063

Merged
metalgearsloth merged 8 commits intospace-wizards:masterfrom
Nopey:fix-clientside-storage-whitelists
Feb 1, 2024
Merged

Fix clientside storage Whitelists#24063
metalgearsloth merged 8 commits intospace-wizards:masterfrom
Nopey:fix-clientside-storage-whitelists

Conversation

@Nopey
Copy link
Copy Markdown
Contributor

@Nopey Nopey commented Jan 14, 2024

About the PR

When a StorageComponent has an EntityWhitelist that filters by a server-only Component, the Client will incorrectly believe items carrying the component do not match the whitelist. This results in a prediction error and erroneous "Can't insert" message.

To demonstrate, try gathering the apples that spawn on Dev with the PlantBag that spawns in the botanist's locker. The following message will appear on master, as the client does not believe Apples are Produce:
image

Additionally, the Assault Belt has been filtering by the pre-Gun Refactor (#8301) "RangedMagazine" component that was been superceded by BallisticAmmoProvider; this meant both server and client would both incorrectly disallow ammo in the assault belt. This is once again allowed:

image

Why / Balance

These prediction errors are confusing to new players, and ignored by experienced players; not pleasant for anyone.
Although the Assault Belt change may be argued to effect gameplay balance, it was clearly not #8301's intent to disallow ammunition from being inserted in the Assault Belt.

Technical details

The following is a list of belts, and what previously-serverside component they filter by.
Security Belt:

  • Flash
  • FlashOnTrigger (as used by flashbangs)
  • SmokeOnTrigger (as used by smoke grenades)

Utility Belt:

  • SprayPainter

Assault Belt:

  • RangedMagazine (has been superceded by BallisticAmmoProvider)

JaniBelt:

  • LightReplacer

Chef Belt:

  • Mousetrap
  • Utensil

Plant Bag:

  • Seed (also in Botanical Belt)
  • Produce

The Botany components (Seed and Produce) now have trivial (empty) implementations in shared and client.
The remaining perviously-serverside components have been moved to the Content.Shared module, with trivial Shared Systems created as-needed for the components' Access property to reference.
Except for Botany, all data fields on the components are present in Shared (and could theoretically now be used in Clientside code).

Media

  • I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

Breaking changes

The following components have been moved from Content.Server to Content.Shared:

  • SprayPainterComponent
  • FlashComponent
  • UtensilComponent
  • LightReplacerComponent
  • SmokeOnTriggerComponent (as used in smoke grenades)
  • FlashOnTriggerComponent (as used in flashbangs)
  • MousetrapComponent

Changelog
🆑

  • fix: Many belts and the plant bag no longer produce an erroneous "Can't insert" and "This doesn't go in there!" messages while inserting certain items such as Produce (plant bag) or Smoke Grenades (security belt).
  • fix: Ammunition can once again be stored in assault belts, as was the case before June 2022.

@Nopey Nopey requested a review from Partmedia as a code owner January 14, 2024 06:25
@github-actions github-actions bot added the S: Needs Review Status: Requires additional reviews before being fully accepted. Not to be replaced by S: Approved. label Jan 14, 2024
Copy link
Copy Markdown
Contributor

@metalgearsloth metalgearsloth left a comment

Choose a reason for hiding this comment

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

Shared components need to be networked.

Also given the number of systems this touches this is very likely going to be conflict bait so might be easier to get through being split up a bit more.

@Nopey
Copy link
Copy Markdown
Contributor Author

Nopey commented Jan 14, 2024

Fixed yaml linter warnings:

  • In security.yml, moved repeating from portable flasher's FlashOnTrigger component to TriggerOnProximity, which appears to have been a typo.
  • In spray_painter.yml, the whitelist section has been removed. This section is likely a vestige of the spray painter's development, as the C# datafield that would deserialize it is not in commit history.
  • Re-did the Botany components, with trivial (no members) on the Shared and Client classes; this avoids introducing the seed prototypes to the client and avoids upsetting the linter.
    The interesting members now remain unmoved in Content.Server.

Content.IntegrationTests was either failing because of the previous botany changes, or was a fluke-- it's now passing locally, so I expect the CI pipeline to pass.

Regarding breaking this work up:
Should each of the (now 6) commits be a seperate MR? that seems like it might make the maintainers' job more difficult, but I can do that.
For now I'm going to push my revised changes here and update the description, but I'll check on this MR in the morning and could certainly break it up then.

@Nopey Nopey force-pushed the fix-clientside-storage-whitelists branch from 33747ee to f0c42b7 Compare January 14, 2024 07:45
@Nopey
Copy link
Copy Markdown
Contributor Author

Nopey commented Jan 14, 2024

EDIT: the ViewVariables attribute is part of admin/mapper tool for editing entities, not anything to do with networking.

old message, not really relevant It's unclear to me what networking changes you are requesting.

As an example, the SprayPainterComponent has several members:

  • IsSpraying is annotated with DataField, which if I understand correctly allows it to be (de)serialized. Does IsSpraying need networking?
  • PickedColor, in addition to DataField, has a ViewVariables field; I believe VV performs networking, with the VVAccess.ReadWrite allowing clients to send read and write messages for this field to the server. Only the server references this field.

Am I misunderstanding what ViewVariables does? Is there a(nother) way of performing networking I am unaware of?

Should I prefer the style used in my Botany changes, where fields remain in Content.Server, and Content.Shared/Client gain trivial (empty) implementations of each component? would cause less breakage, although mirrorcult advised in Discord channel #howdoicode that I should try to "move" components to Shared.

@Nopey
Copy link
Copy Markdown
Contributor Author

Nopey commented Jan 14, 2024

Should close issue #22686, unless we want additional safeguards against the behavior described where:

  1. The player attempts to place an item in a gridinv
  2. The client doesn't think the item can go in there
  3. The client doesn't bother telling the server they tried!
    If we want the client to ask the server just-in-case the whitelist is bugged on the client, we could keep 22686 open (or open a new, more specific issue)

@Nopey Nopey requested a review from metalgearsloth January 14, 2024 08:20
@Nopey
Copy link
Copy Markdown
Contributor Author

Nopey commented Jan 14, 2024

Have discovered NetworkedComponent and the wiki 🙃
Going to add it to each shared component, and start thinking about where replication is needed.

NetworkedComponent is sufficient for the creation of components to be replicated, although their fields remain unreplicated.
Is it sufficient to leave the components' fields unreplicated, as no code currently consumes them?
EDIT: thanks for your answer in Discord, leaving fields unreplicated.

Nopey added 6 commits January 14, 2024 04:17
RangedMagazine was replaced with BallisticAmmoProvider in the Gun
refactor (space-wizards#8301)
The PaintableAirlock tag has also been removed, as it was unused &
unnecessary, likely a vestige of spray painter development when the
PaintableAirlock component wasn't in Content.Shared.
This allows the plant bag and botanical belt whitelists to correctly
match produce and seeds on the client, fixing the extraneous "Can't
insert" message that previously appeared.
@Nopey Nopey force-pushed the fix-clientside-storage-whitelists branch from f0c42b7 to 7f56fb9 Compare January 14, 2024 09:25
@Nopey
Copy link
Copy Markdown
Contributor Author

Nopey commented Jan 26, 2024

@metalgearsloth could you have a look at this, again?

@github-actions github-actions bot added the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Feb 1, 2024
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 1, 2024

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

@metalgearsloth
Copy link
Copy Markdown
Contributor

Gonna conflict with #22691 so gonna do that one first.

…orage-whitelists

# Conflicts:
#	Resources/Prototypes/Entities/Clothing/Belt/belts.yml
…orage-whitelists

# Conflicts:
#	Content.Server/Explosion/EntitySystems/SmokeOnTriggerSystem.cs
#	Content.Shared/Explosion/Components/OnTrigger/SmokeOnTriggerComponent.cs
#	Content.Shared/Explosion/EntitySystems/SharedSmokeOnTriggerSystem.cs
#	Content.Shared/Flash/Components/FlashOnTriggerComponent.cs
@github-actions github-actions bot removed the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Feb 1, 2024
@metalgearsloth metalgearsloth enabled auto-merge (squash) February 1, 2024 13:33
@metalgearsloth metalgearsloth merged commit 9cd6e4d into space-wizards:master Feb 1, 2024
@HyperB1 HyperB1 mentioned this pull request Mar 2, 2025
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S: Needs Review Status: Requires additional reviews before being fully accepted. Not to be replaced by S: Approved.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants