-
Notifications
You must be signed in to change notification settings - Fork 10
fix: Make lone wolf perk into toggle #149
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
base: main
Are you sure you want to change the base?
Conversation
| 15200413822669298636: 1710479452, | ||
| 15207095098433440528: 1680496635, | ||
| 15450820022852980103: 1710724046, | ||
| 15480219162363041159: 1730591229, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this is needed as this was automatically changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol yes :) please always include this file with your prs.
JayAndromeda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some comments as to what I think might be the best of both worlds. if you agree just add em to your branch and ill merge it in. my issue i have with a toggle is that usually the 'off' state means no effect in our current philosophy but i do agree with you that it is a two state perk. adding as an options_raw makes it more clear in my opinion to the user that there is a base effect.
| //episode 2 | year 7 | ||
| Perks::AirTrigger => Some(PerkOptionData::toggle()), | ||
| Perks::ClosingTime => Some(PerkOptionData::options(["Base", "Max Effect"].to_vec())), | ||
| Perks::LoneWolf => Some(PerkOptionData::options(["Base", "Alone"].to_vec())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it is a two state perk and it did always bother me about the 'none' option but seeing this pr reminded me of the options_raw perk option as well. It's basically the same as options but doesnt include a none by default so _input.value = 0 is whatever the first value you give is. A toggle in our current UX usually means no effect so thats why i think this option fits a little better than a toggle.
Perks::LoneWolf => Some(PerkOptionData::options_raw(["Base", "Alone"].to_vec())),
| @@ -76,29 +76,24 @@ pub fn year_7_perks() { | |||
| Box::new(|_input: ModifierResponseInput| -> HashMap<u32, i32> { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we've been adding in clamped values simply as a protection from editing the value in the url or otherwise. it would look like this as an end result.
add_sbr(
Perks::LoneWolf,
Box::new(|_input: ModifierResponseInput| -> HashMap<u32, i32> {
let mut stats = HashMap::new();
let val = clamp(_input.value, 0, 1);
let enhance_buff = if _input.is_enhanced { 1 } else { 0 };
stats.insert(
StatHashes::AIRBORNE.into(),
(15 + 2 * enhance_buff) * (val + 1) as i32,
);
stats.insert(
StatHashes::AIM_ASSIST.into(),
(5 + enhance_buff) * (val + 1) as i32,
);
stats
}),
);
add_hmr(
Perks::LoneWolf,
Box::new(
|_input: ModifierResponseInput| -> HandlingModifierResponse {
let val = clamp(_input.value, 0, 1);
let enhance_buff = if _input.is_enhanced { 0.05 } else { 0.0 };
HandlingModifierResponse {
ads_scale: 0.9 - (0.1 * val as f64) - enhance_buff,
..Default::default()
}
},
),
);
| 15200413822669298636: 1710479452, | ||
| 15207095098433440528: 1680496635, | ||
| 15450820022852980103: 1710724046, | ||
| 15480219162363041159: 1730591229, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol yes :) please always include this file with your prs.
Making Lone Wolf into a toggle since it seemingly only has 2 states when foundry shows 3 (None, Base, and Alone).
If it does in fact have 3 states and the perk description is wrong, someone please correct me.