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

Added several variables to make ClumsyComponent more modular for developers. #33715

Merged
merged 6 commits into from
Dec 6, 2024

Conversation

widgetbeck
Copy link
Contributor

@widgetbeck widgetbeck commented Dec 4, 2024

About the PR

Added nine new variables to ClumsyComponent and its system to allow developers to specify that certain clumsy events should not be applied to a given entity, and to allow developers to have custom per-entity "failed" popup message.

Why / Balance

ClumsyComponent does a lot of different things, and sometimes you only want it to do a few of those things. For example, I was developing a species that is incapable of using guns on my server - I added these variables so that I could disable the hypospray, defibrilator, and table bonk events on those entities.
In short, these changes allow developers to find more creative uses for this component, without the hassle that would be separating each event into its own system.

Technical details

Very simple. Added a check to each event that, if that event's boolean is false, skips the event, and changed the hardcoded strings in ClumsySystem to public LocIds that are read from ClumsyComponent.

Media

Requirements

Breaking changes

Changelog

@github-actions github-actions bot added size/M Denotes a PR that changes 100-999 lines. S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Dec 4, 2024
Content.Shared/Clumsy/ClumsySystem.cs Outdated Show resolved Hide resolved
Content.Shared/Clumsy/ClumsyComponent.cs Outdated Show resolved Hide resolved
Content.Shared/Clumsy/ClumsyComponent.cs Outdated Show resolved Hide resolved
@slarticodefast slarticodefast self-assigned this Dec 4, 2024
@slarticodefast slarticodefast added the S: Awaiting Changes Status: Changes are required before another review can happen label Dec 4, 2024
@github-actions github-actions bot added S: Needs Review Status: Requires additional reviews before being fully accepted and removed S: Awaiting Changes Status: Changes are required before another review can happen labels Dec 4, 2024
@ScarKy0 ScarKy0 added P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. T: Cleanup Type: Code clean-up, without being a full refactor or feature D3: Low Difficulty: Some codebase knowledge required. A: Core Tech Area: Underlying core tech for the game and the Github repository. and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Dec 4, 2024
Copy link
Member

@slarticodefast slarticodefast left a comment

Choose a reason for hiding this comment

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

I noticed another thing, please use spaces instead of tabs everywhere:
grafik
grafik
I recommend using an IDE instead of a text editor. That way you will be corrected if you do things conflicting with our code conventions defined in the .editorconfig file.

@widgetbeck
Copy link
Contributor Author

Ahh, I see. I'll fix that. Out of curiosity, is there a reason that this matters functionally? I've been using tabs on all my custom content for our server.

@slarticodefast
Copy link
Member

slarticodefast commented Dec 4, 2024

Consistency reasons mostly, functionally all whitespace should be the same afaik. If you would be using an IDE the tabs would get automatically converted into spaces, so it wouldn't even be possible to mix both. But if you do it in a texteditor and open it in an IDE afterwards the IDE will scream in pain and throw yellow squiggly lines at you :)

Also git sees the tabs and spaces as different code, so if someone would change a few lines including whitespace then it would mess up the diff.

Copy link
Member

@slarticodefast slarticodefast left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you for your first contribution!

@slarticodefast slarticodefast added S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. and removed S: Needs Review Status: Requires additional reviews before being fully accepted labels Dec 4, 2024
@beck-thompson
Copy link
Contributor

us becks like the clumsy system I guess 😆!

@chromiumboy
Copy link
Contributor

Honestly, I feel like this approach of having a separate bool for every possible clumsy event is something that could get out of hand. If someone wants to add clumsiness to a specific tool in the future, for example, that's another bool to add to the list

I'd prefer a system in which clumsy people have a list of clumsiness rules (i.e. prototypes), which get checked against when a potentially clumsy event occurs. If a match is found, the event occurs; any required data regarding the event (e.g., the message to display to the player) can just be stored in the rule prototype

@slarticodefast
Copy link
Member

slarticodefast commented Dec 6, 2024

I'd prefer a system in which clumsy people have a list of clumsiness rules (i.e. prototypes), which get checked against when a potentially clumsy event occurs. If a match is found, the event occurs; any required data regarding the event (e.g., the message to display to the player) can just be stored in the rule prototype

I think that would probably make things unnecessarily complicated in this case. Another approach would be to split the component up into a separate one for each effect, so that you could use composition to select the effects you want. But that again is not really needed in upstream at the moment. A boolean is simple to use and easily adjustable using VV.

@VasilisThePikachu VasilisThePikachu merged commit 6add781 into space-wizards:master Dec 6, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: Core Tech Area: Underlying core tech for the game and the Github repository. D3: Low Difficulty: Some codebase knowledge required. P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. size/M Denotes a PR that changes 100-999 lines. T: Cleanup Type: Code clean-up, without being a full refactor or feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants