Skip to content

Add rose garden features - #935

Open
Ignose wants to merge 8 commits into
mainfrom
rose-garden
Open

Ignose wants to merge 8 commits into
mainfrom
rose-garden

Conversation

@Ignose

@Ignose Ignose commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread src/resources/2026/BlackRoseGarden.ts Outdated
Comment thread src/resources/2026/BlackRoseGarden.ts Outdated
Comment thread src/resources/2026/BlackRoseGarden.ts Outdated
Comment thread src/resources/2026/BlackRoseGarden.ts Outdated
Comment thread src/resources/2026/BlackRoseGarden.ts Outdated
Comment thread src/resources/2026/BlackRoseGarden.ts Outdated
Comment thread src/resources/2026/BlackRoseGarden.ts Outdated
Comment thread src/resources/2026/BlackRoseGarden.ts Outdated
Comment thread src/resources/2026/BlackRoseGarden.ts Outdated
Comment thread src/resources/2026/BlackRoseGarden.ts Outdated
Comment thread src/resources/2026/BlackRoseGarden.ts Outdated
* @param effect the fountain effect you want to gain
* @returns whether you successfully gained the effect
*/
export function useBloodFountain(effect: Effect): boolean {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

optional but cool: make a string union type to represent the effects a la AprilingBandHelmet

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What's the advantage of this?

Comment thread src/resources/2026/BlackRoseGarden.ts Outdated
Comment on lines +122 to +135
const choices = new Map<number, RoseChoice>();

for (const { decision, extras_joined, label } of Object.values(
availableChoiceExtras(),
)) {
const monsterText = label.match(/^Fight (.+) at position \d+,\d+$/);

if (monsterText) continue;

choices.set(decision, {
text: label,
extras: extras_joined,
});
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

to clarify: when I said "this all can be a .map call", I meant that you could do

const choices = Object.values(availableChoiceExtras).map(({ decision, extras_joined, label }) => stuff);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I didn't make this change because I don't understand how this works any differently than what I've got.

@spaghetti-squash spaghetti-squash left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two more general comments that can't really be targeted at a specific thing:

  1. This is failing linting because you need to bump the VERIFY_CONSTANTS_SINCE value.
  2. I think this operates in a moderately counterintuitive and potentially problematic but probably fine way, and that is that it "enters" the garden implicitly in the getChoices function. This is weird because it means we have a really important side-effect hidden in a function whose name suggests it's purely informational. I would look at the implementation of AutumnAton and ChestMimic for design patterns that handle the situation more appropriately. In particular, many of these functions will leave the user "in the garden", which can interfere with handlingChoice checks elsewhere in the code. That part we need to resolve regardless of whether we plan to restructure the getChoices situation


if (!fountain) return false;

runChoice(fountain.decision, `${fountain.extras}&option=${buff.option}`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a very odd runChoice call, are you sure it's correct? You're essentially double-sending the option URL parameter, first as fountain.decision and second as buff.option. I suspect we want to do two consecutive runChoices, the first with fountain.decision and fountain.extras, and the second time with just buff.option; when I navigated around in my browser, the two calls I got were

whichchoice=1637&pwd=REDACTED&option=1&rgpoi=2&rgx=16&rgy=19&rgf=1
pwd=REDACTED&whichchoice=1639&option=1

FOUNTAIN_OPTIONS.map((ef) => toEffect(ef)),
);

export type FountainEffect = (typeof FOUNTAIN_EFFECTS)[number];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This type is identical to Effect. It does nothing. Look at the types used by functions like AprilingBandHelmet.joinSection or AprilingBandHelmet.changeSong; they accept a string union type (i.e., (typeof FOUNTAIN_OPTIONS)[number] unioned with a MafiaClass (i.e., Effect). All you've done here is create an alias for effect

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The fuller thing you need to do is:

  1. update this type to reference FOUNTAIN_OPTIONS, giving you the union type you need
  2. Either throw a | Effect at the end of this type or change the input type of useBloodFountain to be FountainEffect | Effect, dealer's choice.
  3. either way, add some quick little step to useBloodFountain where we take the input and use some quick ternary (either with typeof or instanceof, dealer's choice) to map both cases to an Effect.

/**
* @returns whether you successfully retrieved all free rose garden items
*/
export function obtainItems(): boolean {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it worth returning the items we obtain, or is that not really practical? I think obtaining items is essentially costless, so I can understand having just a "get them all" function with no "what are they" function, but it still feels like the results would be a better return value than just a boolean.

This branch has not been deployed

No deployments
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