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

More robust test for multi material portions #55461

Merged

Conversation

bombasticSlacks
Copy link
Contributor

@bombasticSlacks bombasticSlacks commented Feb 17, 2022

Summary

None

Purpose of change

Should de-flake the recent test in #55405

Describe the solution

test now tests against the identical armor but if it were broken. So if they aren't basically identical the code is working.

Also to make this pass I fixed a potential divide by 0

Describe alternatives you've considered

Testing

If it passes it should be good.

Additional context

@github-actions github-actions bot added json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Feb 17, 2022
@BrettDong
Copy link
Member

Seems the UBSan test fails because of a division-by-zero error at

for( armor_portion_data &it : obj.armor->data ) {
for( part_material &mat : it.materials ) {
// scale the value of portion covered based on how much total is covered
// if you proportionally only cover 5% of the arm but overall cover 50%
// you actually proportionally cover 10% of the armor
mat.cover = static_cast<float>( mat.cover ) / ( static_cast<float>( it.coverage ) / 100.0f );
}
}

@bombasticSlacks
Copy link
Contributor Author

bombasticSlacks commented Feb 18, 2022

Seems the UBSan test fails because of a division-by-zero error at

Thanks for letting me know I'll PR a fix.

EDIT: Ahh I see it's just happening with this new armor, so I'll fix it here actually thanks

@BrettDong
Copy link
Member

The test still fails
!(5.9342f == Approx( 5.9210000038 ))

@bombasticSlacks
Copy link
Contributor Author

Alright I think the issue was the armor values were small enough that since damage is bottomed/rounded to an int when applied to hp in a lot of cases the small differences between the two armors weren't enough to get to the next whole number. The increased thickness should effect it enough to make this reliable.

@kevingranade kevingranade merged commit 90b1c80 into CleverRaven:master Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants