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

RandomBMMixed description text contains a list of excluded skills, and this is confusing #3015

Open
cgolubi1 opened this issue Nov 15, 2024 · 1 comment
Labels

Comments

@cgolubi1
Copy link
Contributor

cgolubi1 commented Nov 15, 2024

We had a discussion on discord about https://github.com/buttonmen-dev/buttonmen/pull/3013/files#diff-4c9ca62c93b83d35d7afd15b86503e5936f7df91eacb31e57ed196f097c3946f --- the change to the RandomBMMixed flavor text as part of #2844. This is kind of a bug, and definitely confusing to people who are thinking about the impact of this PR in isolation (i suspect not that confusing to actual players, so i don't think it's a blocking bug).

The issue is that Mad and Mood skills still can't appear on RandomBMMixed --- but now they can't appear because those skills only appear on swing dice and that button has no swing dice, not because they are explicitly "excluded." So technically the new flavor text is correct, but it's definitely at least arguably misleading.

The correct fix is to update the RandomBMMixed flavor text so that it no longer contains a list of excluded skills. This is the correct fix because:

  • It's not particularly valuable to have that list in the flavor text --- none of the other RandomBMs have it, e.g. RandomBMDuoSkill ("This button gets a different random recipe in each game, with four regular dice and one swing die, and 2 skills each appearing a total of 2 times on various dice."), and to my knowledge no one has ever complained. Anyone who cares that much is probably already able to look at the source code, which is moderately well-documented
  • It is moderately expensive to maintain that list --- there's code to do it, we have to adjudicate issues like this change, and anything that shows up in test code has to be wrangled around replay testing every time we change the relevant code.
  • It is leading to this conversation, where because the handling of Mad/Mood is ambiguous in a way that doesn't necessarily match the term "excluded," we have to figure out what the right fix it.

So: basically zero benefit + dev cost + friction cost = let's just update the text to be generic like all the other ones, and have done.

@cgolubi1
Copy link
Contributor Author

I don't have time to do this in an emergency for this weekend's release, and i still assert this is non-blocking for #3013 to go to prod:

  • Barring objections (or more critical bugs being found), i'll go ahead with the release to prod of Enable Mad and Mood skills in RandomBM #2844 on Sunday, and fix this next week
  • If people do think this is blocking, we'll have to skip the prod release and block other development until it can be fixed for next week

@cgolubi1 cgolubi1 added the bug label Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant