-
Notifications
You must be signed in to change notification settings - Fork 46
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
oopsy: add new helpers for missed/multiple events #475
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for putting all that together. I think that's a reasonable direction for us to take this. I don't have a ton to say about it right now since I haven't touched the Oopsy internals for a while, but definitely continue with it this way and let's see where it lands.
// Ignore TODOs from `util/sync_files.ts` that haven't been filled out. | ||
const abilityId = typeof detail === 'string' ? detail : detail.id; |
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.
This definition should probably go before the comment for slightly easier readability.
for (const value of Object.values(set.damageWarn ?? {})) | ||
this.mistakeDamageMap[value] = 'warn'; | ||
this.mistakeDamageMap[typeof value === 'object' ? value.id : value] = 'warn'; |
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 don't feel particularly strongly about this, but do we maybe want to break the evaluation and assignment into multiple lines, just to make it slightly more readable at the expense of vertical height?
It's maybe a little unfortunate that the map structures don't really allow us to group the warning and fail maps to loop through those groups instead. I can't think of a better way to structure this right now, but I wonder whether there's something we could do to reduce the Giant Loop Tower somehow, since otherwise it will only grow if we add anything new.
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.
Something like this?
// @TODO: Add to this type guard to cover the MistakeDetails class too.
const isMistakeMap = (v: unknown): v is MistakeMap => {
if (typeof v !== 'object' || v === null)
return false;
if (!Object.keys(v).every((k) => typeof k === 'string')) {
return false;
}
if (!Object.values(v).every((v) => typeof v === 'string')) {
return false;
}
return true;
};
const extractMistakeMaps = (set: ProcessedOopsyTriggerSet): { [key: string]: MistakeMap } => {
const ret: { [key: string]: MistakeMap } = {};
for (const entry of Object.entries(set)) {
const key = entry[0];
if (typeof key !== 'string')
continue;
const value = entry[1];
if (isMistakeMap(value)) {
ret[key] = value;
}
}
return ret;
};
// ...
PushTriggerSet(set: ProcessedOopsyTriggerSet): void {
this.triggerSets.push(set);
for (const set of this.triggerSets) {
for (const mapEntry of Object.entries(extractMistakeMaps(set))) {
const warnFail = mapEntry[0].endsWith('Warn') ? 'warn' : 'fail';
const map = mapEntry[1];
for (const value of Object.values(map)) {
const key = typeof value === 'object' ? value.id : value;
this.mistakeDamageMap[key] = warnFail;
}
}
}
}
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.
Yeah, that seems reasonable. It's maybe a little bit more to read and absorb, but should require less overall maintenance in future, I think
Thanks for the feedback! And yeah, @valarnin 's suggestion looks good to me too. I'll probably wait to pick this back up until after we get thru 7.11 updates next week, but I think this is fairly close, just needs a little more in-game testing. |
This PR adds some new helpers to oopsy, specifically:
(These helpers are all on the current oopsy to-do list).
To implement these helpers, this PR also extends the current
MistakeMap
type in oopsy to now accept params instead of just an action/effect id -- among other things, to make it possible to limit mistakes to particular roles, control the amount of time a missed/multiple mistake will collect for, override the default mistake text, etc.Marking this as a draft for now because it still needs more in-game testing and some docs updates. Would greatly appreciate any early feedback though.