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

Add global constants for damage roll related code and make AverageRollDmg faster #4663

Merged
merged 3 commits into from
May 31, 2024

Conversation

Sneed69
Copy link

@Sneed69 Sneed69 commented May 30, 2024

Discord contact info

duke5614

@Pawkkie
Copy link
Collaborator

Pawkkie commented May 30, 2024

I should note that the initial intent for these boundaries was specifically to give the user control over how the AI perceives damage rolls in their calculations, and not affect the calculations of actual damage themselves. Using the same constants in the DoDamage calcs removes this feature, which was its intended use.

@Sneed69
Copy link
Author

Sneed69 commented May 30, 2024

That seems like a strange design choice, but I could restore it.

@Pawkkie
Copy link
Collaborator

Pawkkie commented May 30, 2024

It's for users who would prefer the AI behave more like a human.

A human running damage calcs in a competitive game is very unlikely to consider the highest rolls in the vast majority of cases because of how unlikely they are, and is more likely to make decisions around KO thresholds based off say a 66% or a 50% damage roll threshold most of the time.

This lets users have the AI emulate those lines of thinking without altering the actual damage calculation. I'm on board for adding the new defines that do directly affect the damage calculation, that's an interesting knob to control, but I'd keep them completely separate from how the AI manages its rolls when making decisions.

@Sneed69
Copy link
Author

Sneed69 commented May 30, 2024

You made it use the average roll by default and added the conservative flag. Do these not do the same thing?

@Pawkkie
Copy link
Collaborator

Pawkkie commented May 30, 2024

Separately, I'm not sure the change to speed up AverageRollDmg is worth the reduced futureproofing if GF or anyone else changes how min and max rolls get calculated, as that wouldn't require touching this function as is and would require changing it after the fact. The change in speed is imperceptible, the AI's thinking delay is the same frame count with either implementation. Though frankly changing AverageRollDmg manually for some future change is not a big deal either.

I'm not sure whether it's considered better practice to futureproof or maximize efficiency, but I thought it worth mentioning that there is a difference in what approach is preferred here :)

@Pawkkie
Copy link
Collaborator

Pawkkie commented May 30, 2024

You made it use the average roll by default and added the conservative flag. Do these not do the same thing?

No, because the conservative flag uses lowrolls and the average flag uses an exactly average roll. If you don't want the AI to consider say the upper third of results because rolls only have a ~33% chance of falling in that range, you can adjust their thinking manually by setting the upper bound constant to 95 and the lower bound to 85.

This supports all of the permutations of AI behaviour, risky, standard and conservative, but gives flexibility on what roll bounds the AI uses across the board for the reasons I mentioned above, again without actually impacting the damage itself.

dmg /= 100;
return dmg;
}

static inline s32 AverageRollDmg(s32 dmg)
{
dmg = ((HighestRollDmg(dmg) + LowestRollDmg(dmg)) * 100) / 2;
dmg = (dmg * (DMG_ROLL_PERCENT_LO + DMG_ROLL_PERCENT_HI)) / 2;
Copy link
Collaborator

@AlexOn1ine AlexOn1ine May 31, 2024

Choose a reason for hiding this comment

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

Could you change the equation to always result in the 9th roll of the damage? I brought it up before and ultimately thought it didn't matter much but I think it is crucial for the debug menu to have the correct value shown. Right now the calc could end up in a non existing roll.

Copy link
Author

Choose a reason for hiding this comment

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

I have absolutely no idea what this means.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The current calculation calcs an average but the average can end up between roll 8 and 9. This doesn't matter for the actual damage calculations but it will affect the values in the debug menu.

Copy link
Author

Choose a reason for hiding this comment

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

I still do not follow. What is a "roll 8" and "roll 9?"

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

@AlexOn1ine AlexOn1ine merged commit e869aaf into rh-hideout:upcoming May 31, 2024
1 check passed
@Sneed69 Sneed69 deleted the damage-roll-stuff branch June 2, 2024 19:05
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