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

Random normal always random #10165

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

magralo
Copy link

@magralo magralo commented Feb 10, 2024

Trying to solve #10164

This works at least for my usage!

You will need to select a target but the target will be reselected randomly in the backend,

Maybe not the best solution but this works for me... Hope this can merge with master

@DaWoblefet
Copy link
Member

DaWoblefet commented Feb 10, 2024

The way moves like Outrage currently work is like this:

When not locked into Outrage: send no target argument (i.e. /move outrage, other action).
When locked into Outrage: send either target as an argument (i.e. /move outrage +2, other action or /move outrage +1, other action).

The target ends up getting randomized no matter what here already. I don't think we should actually be doing it in the way you're doing so; you shouldn't need to send a target for randomNormal moves. The fix would be requiring that you don't supply a target, not that you do supply one.

@magralo
Copy link
Author

magralo commented Feb 20, 2024

Ok thanks!

... one question, though: Wouldnt it be better if the move never required a target?

@DaWoblefet
Copy link
Member

DaWoblefet commented Feb 21, 2024

Correct, that's what I'm saying. For randomNormal moves, we shouldn't accept a target argument. Right now, randomNormal moves are already being randomized - the target argument isn't being respected right now anyway, which is correct behavior. It should just throw an error if you try to provide one.

@DaWoblefet DaWoblefet marked this pull request as draft March 22, 2024 02:19
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