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.

Sign up for GitHub

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

Jump to bottom

SpiderSystem refactor and rename #33742

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

SpiderSystem refactor and rename #33742

wants to merge 16 commits into from

Conversation

iaada
Copy link

@iaada iaada commented Dec 6, 2024 •
edited
Loading

About the PR

Refactor of the component and systems that give spiders the ability to spawn webs, and renamed them to a less generic name. Changes include:

  • Webs no longer spawn on webs. (bugfix)
  • Webs no longer spawn in space.
  • Play a sound when webs spawn.
  • More yaml control in where webs spawn (still somewhat limited).

Why / Balance

The only balance change is that spiderwebs no longer spawn on other spiderwebs. This is a bugfix though, as the code to do this already existed and just didn't work.

Technical details

SpiderSystem, SharedSpiderSystem and SpiderComponent renamed to WebPlacerComponent, SharedWebPlacerSystem, and WebPlacerSystem.
SpiderWebObjectComponent deleted. It mostly existed to be found by a whitelist in yaml, and has been replaced with a tag.
SpiderWebVisuals enumerator deleted. This was used alongside SpiderWebObjectComponent to give webs a random appearance, and has been replaced with RandomSpriteComponent and a generic enum.
Minor yaml changes. Changed the renamed component, replaced the deleted component. Clown spiders have a honk horn sound.

Media

None.

Requirements

Breaking changes

SpiderComponent renamed to WebPlacerComponent.
SharedSpiderSystem renamed to SharedWebPlacerSystem.
SpiderSystem renamed to WebPlacerSystem.
Namespace Content.Shared.Spider changed to Content.Shared.WebPlacer.
Namespace Content.Server.Spider changed to Content.Server.WebPlacer.
SpiderWebObjectComponent removed. Replaced with Tag: SpiderWeb.
IgnoreSpiderWebComponent moved to namespace Content.Shared.IgnoreSpiderWeb.
SpiderWebVisuals enumerator removed.

Changelog

🆑 aada

  • fix: Spiderwebs no longer spawn over other spider webs.
  • tweak: Spiderwebs no longer spawn in space.

@github-actions github-actions bot added S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. size/L Denotes a PR that changes 1000-4999 lines. labels Dec 6, 2024
@Boaz1111 Boaz1111 added P3: Standard Priority: Default priority for repository items. T: Refactor Type: Refactor of notable amount of codebase D2: Medium Difficulty: A good amount of codebase knowledge required. A: Core Tech Area: Underlying core tech for the game and the Github repository. A: Roundflow/Antag Area: Roundflow - "What happens in the game", including antagonist roles and their capabilities and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. A: Core Tech Area: Underlying core tech for the game and the Github repository. labels Dec 6, 2024
@BolloTea
Copy link

BolloTea commented Dec 6, 2024

Could you please leave the web overlay or put restrictions on web overlays if it is a big problem?

Aeshus
Aeshus requested changes Dec 6, 2024
View reviewed changes
Copy link
Contributor

@Aeshus Aeshus left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I just have some nitpicks.

Content.Server/WebPlacer/WebPlacerSystem.cs Outdated Show resolved Hide resolved
Content.Server/WebPlacer/WebPlacerSystem.cs Outdated Show resolved Hide resolved
Content.Server/WebPlacer/WebPlacerSystem.cs Outdated Show resolved Hide resolved
Content.Shared/WebPlacer/WebPlacerComponent.cs Outdated Show resolved Hide resolved
@0x6273
Copy link
Contributor

0x6273 commented Dec 6, 2024

SpiderWebObjectComponent and IgnoreSpiderWebComponent deleted. Both of these existed to be found by a whitelist in yaml, and have been replaced with tags.

You can use components in whitelists as well as tags. What's the reason for changing them to be tags?

@iaada iaada requested a review from Aeshus December 9, 2024 04:13
@github-actions github-actions bot added the S: Needs Review Status: Requires additional reviews before being fully accepted label Dec 9, 2024
@iaada
Copy link
Author

iaada commented Dec 9, 2024 •
edited
Loading

You can use components in whitelists as well as tags. What's the reason for changing them to be tags?

It's because they were empty components. They didn't do anything except exist to be found by a whitelist, which is what tags are for.

@0x6273
Copy link
Contributor

0x6273 commented Dec 9, 2024

You can use components in whitelists as well as tags. What's the reason for changing them to be tags?

It's because they were empty components. They didn't do anything except exist to be found by a whitelist, which is what tags are for.

But there's no real benefit to using tags over marker comps here. If fact I'm pretty sure tags are slower since it needs to both get the tag component and check if it has the tag, vs just checking for a comp.

Tags can't be used in queries either, so if a downstream relies on querying these comps they would either have to skip or partially merge this PR, or make their code slower and more complicated.

Aeshus
Aeshus reviewed Dec 12, 2024
View reviewed changes
Copy link
Contributor

@Aeshus Aeshus left a comment

Choose a reason for hiding this comment

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

Thanks for making the requested changes!

In return, you get some more nitpicks! ;)

Content.Shared/WebPlacer/WebPlacerComponent.cs Outdated Show resolved Hide resolved
Content.Shared/WebPlacer/WebPlacerComponent.cs Outdated Show resolved Hide resolved
Content.Shared/WebPlacer/WebPlacerComponent.cs Outdated Show resolved Hide resolved
@Tayrtahn
Copy link
Member

But there's no real benefit to using tags over marker comps here. If fact I'm pretty sure tags are slower since it needs to both get the tag component and check if it has the tag, vs just checking for a comp.

Tags can't be used in queries either, so if a downstream relies on querying these comps they would either have to skip or partially merge this PR, or make their code slower and more complicated.

Agreed. Marker components are common in ECS and avoid a few issues that come up with the way Tags work (like inheritance). I endorse sticking with using marker components here.

@iaada
Copy link
Author

iaada commented Dec 16, 2024

I can see your points with IgnoreSpiderWeb. Considering how often it gets checked and it being needed by mobs with several tags already, it's probably best to leave it as a marker component. I've re-added it. SpiderWebObject however is not inherited anywhere and only queried when spawning the webs every 25 seconds, and has no inherent functionality. It feels like prime use case for a tag.

@iaada iaada requested a review from Aeshus December 16, 2024 03:44
Aeshus
Aeshus requested changes Dec 22, 2024
View reviewed changes
Copy link
Contributor

@Aeshus Aeshus left a comment

Choose a reason for hiding this comment

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

Thanks for making the requested changes!

I just have some more comments and changes, but the PR is definitely getting to the state of being ready to be merged!

Content.Shared/WebPlacer/SharedWebPlacerSystem.cs Outdated Show resolved Hide resolved
Content.Shared/WebPlacer/WebPlacerComponent.cs Outdated Show resolved Hide resolved
Resources/Prototypes/Actions/spider.yml Outdated Show resolved Hide resolved
Resources/Prototypes/Entities/Objects/Misc/spider_web.yml Outdated Show resolved Hide resolved
Content.Shared/IgnoreSpiderWeb/IgnoreSpiderWebComponent.cs Outdated Show resolved Hide resolved
TheShuEd
TheShuEd requested changes Dec 22, 2024
View reviewed changes
Copy link
Member

@TheShuEd TheShuEd left a comment

Choose a reason for hiding this comment

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

Codestyle review

Content.Server/WebPlacer/WebPlacerSystem.cs Outdated Show resolved Hide resolved
Content.Server/WebPlacer/WebPlacerSystem.cs Outdated Show resolved Hide resolved
Content.Server/WebPlacer/WebPlacerSystem.cs Outdated Show resolved Hide resolved
Content.Shared/WebPlacer/SharedWebPlacerSystem.cs Outdated Show resolved Hide resolved
Content.Shared/WebPlacer/WebPlacerComponent.cs Outdated
/// <summary>
/// Action given to the player.
/// </summary>
//[ViewVariables]
Copy link
Member

Choose a reason for hiding this comment

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

why its commented? Where DataField

Copy link
Author

Choose a reason for hiding this comment

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

I didn't add DataField because this variable is getting set on init, and to my understanding EntityUid can't be set in .yml so it felt unnecessary.
VV being commented was just a mistake on my part.

@github-actions github-actions bot added size/M Denotes a PR that changes 100-999 lines. and removed size/L Denotes a PR that changes 1000-4999 lines. labels Dec 30, 2024
@iaada iaada requested review from TheShuEd and Aeshus December 30, 2024 02:09
@iaada iaada requested a review from Aeshus December 30, 2024 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@witchesfall witchesfall witchesfall approved these changes

@TheShuEd TheShuEd Awaiting requested review from TheShuEd

@Aeshus Aeshus Awaiting requested review from Aeshus

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
A: Roundflow/Antag Area: Roundflow - "What happens in the game", including antagonist roles and their capabilities D2: Medium Difficulty: A good amount of codebase knowledge required. P3: Standard Priority: Default priority for repository items. S: Needs Review Status: Requires additional reviews before being fully accepted size/M Denotes a PR that changes 100-999 lines. T: Refactor Type: Refactor of notable amount of codebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@iaada @BolloTea @0x6273 @Tayrtahn @Aeshus @TheShuEd @witchesfall @Boaz1111