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

Feat additional manipulate functions #12

Draft
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

Darxo
Copy link
Contributor

@Darxo Darxo commented Jan 3, 2024

These are two minor ideas for the perk group class that can improve submodding, when one of the mods wants to change things done by another.

@Darxo Darxo requested a review from LordMidas January 3, 2024 12:24
@Darxo Darxo force-pushed the feat-additional-manipulate-functions branch from 7f1ee2c to 12656cb Compare January 3, 2024 13:50
@LordMidas
Copy link
Contributor

Definitely seems like a good idea to return the tier of the perk when removing from a perk group. I'm not completely convinced on the replacePerk function because how would that work in practice in terms of achieving a goal. If my goal is to replace perk A with perk B in a perk group:

perkGroup.replacePerk(perkA, perkB); 

seems fine but then I want to know whether the perk actually got replaced and if not then I need to add it myself so in fact I still end up doing something like:

local tier = perkGroup.removePerk(perkA);
perkGroup.addPerk(perkB, tier == null ? 4 : tier);

In order to get around this we could add an optional parameter _tier = null to the replacePerk function e.g.:

perkGroup.replacePerk(perkA, perkB, 4);

If you don't pass the tier, then the function only tries to replace the perk and if perkA isn't found, then perkB isn't added. If you pass a tier, then if perkA isn't found then the function adds perkB to the given tier. If perkA is found then the passed _tier is ignored and perkB is added to the same tier at which perkA existed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants