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

support snapper as a familiar #1578

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

pstalcup
Copy link
Contributor

No description provided.

@pstalcup pstalcup marked this pull request as ready for review September 19, 2023 15:01
Copy link
Contributor

@horrible-little-slime horrible-little-slime left a comment

Choose a reason for hiding this comment

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

Tentative approval. I'm concerned there may be other queue manipulators not being accounted for in your python script--it doesn't look like it checks for both the turtlesexy and the turtlesexless. But also I think we can just assume people have turtlesex; it's 300k and an autoperm, apparently

Comment on lines +693 to +702
export function barfEncounterRate(options: {
snapper?: boolean;
snapperPhylum?: Phylum;
olfact?: Monster;
longCon?: Monster;
motif?: Monster;
turtle?: Monster;
monkeyPoint?: Monster;
humanity?: boolean;
}): Map<Monster, number> {
Copy link
Contributor

Choose a reason for hiding this comment

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

just make this a Partial

src/lib.ts Show resolved Hide resolved
Comment on lines +736 to +737
const copies = (target: Monster | null, n: number): Monster[] =>
n === 0 ? [] : [...zoneMonsters.filter((m) => m === target), ...copies(target, n - 1)];
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like a .fill solution will be more performant and legible than the recursive one

const copies = (target: Monster | null, n: number): Monster[] =>
n === 0 ? [] : [...zoneMonsters.filter((m) => m === target), ...copies(target, n - 1)];

const monsterQueue = [
Copy link
Contributor

Choose a reason for hiding this comment

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

call this the CSV, not the queue. Or call it some other thing. But don't call it the queue

Copy link
Contributor

Choose a reason for hiding this comment

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

monsterPool?

Comment on lines +732 to +734
if (cachedValue) {
return cachedValue;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (cachedValue) {
return cachedValue;
}
if (cachedValue) return cachedValue;

...copies(monkeyPoint, 2),
...zoneMonsters
.filter((m) => snapper && m.phylum === snapperPhylum)
.flatMap((m) => copies(m, 2)),
Copy link
Contributor

Choose a reason for hiding this comment

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

I have had trouble with polyfilling flatMap before; definitely make sure this works the way you want it to

const encounters = monsterQueue.flatMap((m) =>
monsterQueue.map((n) =>
// olfaction, longcon, and motif caus that monster to ignore queue rejection
olfact === m || longCon === m || motif === m || !encounterQueue.includes(m) ? m : n,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
olfact === m || longCon === m || motif === m || !encounterQueue.includes(m) ? m : n,
[olfact, longCon, motif].includes(m) && !encounterQueue.includes(m) ? m : n,


const zoneMonsters = $monsters`garbage tourist, angry tourist, horrible tourist family`;
const encounterQueue = zoneMonsters.filter((m) =>
$location`Barf Mountain`.combatQueue.includes(`${m}`),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I prefer m.name here but I think using the template literal below makes sense so maybe being consistent within this area is better

.flatMap((m) => copies(m, 2)),
];

const encounters = monsterQueue.flatMap((m) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what this variable is meant to represent

Copy link
Contributor

Choose a reason for hiding this comment

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

update: I think I might understand it, but if I do then it's not quite right: we only reroll 75% of the time, but even a reroll can reroll.

This math is really hard to do--slaw wrote a big ol' thesis about it. I think we have to do it numerically instead of analytically.

@horrible-little-slime
Copy link
Contributor

I know that you're working on a whole new thing for this, but just as a reminder one decision we made in discord is that alongside this we should always print marginal familiar printouts in barf, not just when we pick marginal over it, and for snapper specifically we should add a based on queue-state message or something that helps users understand why garbo keeps "changing its mind"

Comment on lines +89 to +93
const dudeRate = [...encounterRate.entries()].reduce(
(acc: number, entry: [Monster, number]) =>
entry[0].phylum === $phylum`dude` ? entry[1] + acc : acc,
0,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const dudeRate = [...encounterRate.entries()].reduce(
(acc: number, entry: [Monster, number]) =>
entry[0].phylum === $phylum`dude` ? entry[1] + acc : acc,
0,
);
const dudeRate = sum([...encounterRate.entries()], ([monster, weight]) => monster.phylum === $phylum`dude` ? weight : 0);

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.

3 participants