-
Notifications
You must be signed in to change notification settings - Fork 16
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
Dry run fixes #29
Dry run fixes #29
Conversation
… Frozen Core removing mana
export const FRIENDLY_CHANCES = { | ||
W9: FROZEN_CORE_STAYS, | ||
S3: AHMI_RETURNS, | ||
W16: DAWNSPARKS_HIT * DAWNSPARKS_STAYS, | ||
} |
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.
Very interesting, I like the idea of having this outside of the code itself for better clarity. Maybe we could make this list more explicit like this:
const RNG_DETAILS = {
W9: {
FRIENDLY: () => true,
REGULAR: () => Math.random() > 0.5,
UNFRIENDLY: () => false
},
// …
}
Then we could access the chances like so:
const shouldFrozenCoreStay = RNG_DETAILS[card.id][this.state.RNG]()
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 moved the calculations to a helper function - This might no longer be needed
@@ -16,13 +27,13 @@ export default class DeckMechanisms extends React.Component { | |||
constructor(props) { | |||
super(props) | |||
|
|||
this.state = { | |||
this.DEFAULT_STATE = { |
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.
You should move this out of the class itself.
const DEFAULT_STATE = { … }
export default class DeckMechanisms extends React.Component {
constructor(props) {
super(props)
this.state = { ...DEFAULT_STATE }
}
}
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.
That's what I initially did, but I needed props to define DEFAULT_STATE
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.
You could make it a function then.
const getDefaultProps = () => ({ … })
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 tried to do so, not sure this is what you meant
Co-Authored-By: Kitty <53430374+KittySparkles@users.noreply.github.com>
Co-Authored-By: Kitty <53430374+KittySparkles@users.noreply.github.com>
…d-kitty into dry-run-fixes
const getBinomialRandomVariableResult = (n, p) => { | ||
// Iterate over n objects, keep each one with probability p | ||
// Return how many objects were kept |
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.
const getBinomialRandomVariableResult = (n, p) => { | |
// Iterate over n objects, keep each one with probability p | |
// Return how many objects were kept | |
// Iterate over n items, keep each one with probability | |
// @param {Number} length - Number of items | |
// @param {Float} probability - Probability | |
// @return Number of items that were kept | |
const getBinomialRandomVariableResult = (length, probability) => { |
this.state.RNG === 'REGULAR' | ||
? getBinomialRandomVariableResult(activeDawnsparks, DAWNSPARKS_HITS) |
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 feel like this should be done in switch; that’s why we have the switch in the first place.
case 'REGULAR': {
// …
state.mana += getBinomialRandomVariableResult(activeDawnsparks, DAWNSPARKS_HITS) * 4
// …
break
}
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 wanted to add mana from FC and Dawnsparks in the same place for clarity, but I'll move it
sequence_length, | ||
success_probability |
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.
We tend to use camelCase in JavaScript, so maybe you could change them for sequenceLength
and successProbability
. But otherwise it looks super nice. 👍
@@ -422,8 +449,8 @@ export default class DeckMechanisms extends React.Component { | |||
} | |||
|
|||
if (this.state.turn === 1) { | |||
const unplayableSpells = ['W1', 'S10', 'N15'] | |||
|
|||
const unplayableSpells = ['W1', 'S10', 'N15', 'N63'] |
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.
Let’s remove that change from this pull-request and do it separately with a test? :)
@@ -361,36 +411,13 @@ export default class DeckMechanisms extends React.Component { | |||
// Reset the mana to 3 + the current turn | |||
newState.mana = DEFAULT_MANA + state.turn | |||
|
|||
// Reset the cycling state | |||
// Reset the cycling and freezing state |
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 think the comment is correct?
getBinomialRandomVariableResult( | ||
state.specifics.activeDawnsparks, | ||
DAWNSPARKS_HITS | ||
) * 4 |
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.
How comes you are recomputing it a second time? It seems you did it once on L382 already.
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.
Once to remove Dawnsparks that were killed by the opponent, once to calculate how many of those who survived actually hit an enemy unit and give mana
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.
Ah yes, of course!
Added Unhealthy Hysteria