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

MindRemoveRole refactor #34880

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Errant-4
Copy link
Member

@Errant-4 Errant-4 commented Feb 4, 2025

About the PR

Reorganised MindRemoveRole and it's overloads. Mind roles can now be removed based on prototypeId

Why / Balance

Need this for an upcoming PR.
Until now, it was only possible to remove them by specifying a component. Now the same data that can create them can also remove them.

Technical details

MindRemoveRole can now be called without , instead specifying a protoId as the second parameter.
Both the old and the new version runs their own foreach on the mind's roles, and creates a deletion list. They then call the new MindRemoveRoleDo(), which contains parts of the code that could be shared between the different versions.
The Found variable which was used to mark that deletable entities were found has been removed, in favor of simply checking if the Delete list contains any elements.

Requirements

Breaking changes

MindTryRemoveRole() was replaced with an overload of MindRemoveRole(). Simply remove "Try" from every existing use, and it will work.

Changelog

No player or admin facing changes.

@github-actions github-actions bot added S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. size/S Denotes a PR that changes 10-99 lines. labels Feb 4, 2025
@Errant-4 Errant-4 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. S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. 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 Feb 4, 2025
@github-actions github-actions bot added the S: Needs Review Status: Requires additional reviews before being fully accepted label Feb 5, 2025
@slarticodefast
Copy link
Member

Who dares summon me?

/// <param name="mind">The mind entity and component</param>
/// /// <param name="protoId">The prototype ID of the mind role to be removed</param>
/// <returns>True if the role existed and was removed</returns>
public bool MindRemoveRole(Entity<MindComponent?> mind, EntProtoId<MindRoleComponent> protoId)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if removing components depending on a prototype is the right approach. Usually we don't want prototypes to be used for anything other than spawning entities.
What is the usecase you will need this for?

Copy link
Member Author

@Errant-4 Errant-4 Feb 5, 2025

Choose a reason for hiding this comment

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

Specifying a temporary mindrole by datafield.
Specific usecase is making a brain/pAI insertable into a borg ((whether this is a desired feature is not relevant for the purposes of functionality)). It must get the Silicon mindrole when put into a borg, and lose it when removed. The most straightforward way for this is to specify the prototypeID in yaml, and use it both for adding and removal.
The alternative would be to store some type of "termination tag/identifier" on each MindRoleComponent, then you would have to Index the prototypeID supplied for creation, read it's MindRoleComponent, and search the target's mind role entities for that, and remove. That was how I was first going to do it, but it seems needlessly complicated

Copy link
Member

Choose a reason for hiding this comment

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

Using prototypes for this looks still a little sus, but the idea makes sense.
We best ask sloth if he can think of a different approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

My backup idea would be to use Tags, but then I would have to make a bunch of tags, seems like a waste of time and space

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. D2: Medium Difficulty: A good amount of codebase knowledge required. P3: Standard Priority: Default priority for repository items. S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. S: Needs Review Status: Requires additional reviews before being fully accepted size/S Denotes a PR that changes 10-99 lines. T: Refactor Type: Refactor of notable amount of codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants