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

Frag - Add public blacklist interface #10163

Merged
merged 364 commits into from
Sep 18, 2024

Conversation

lambdatiger
Copy link
Contributor

@lambdatiger lambdatiger commented Aug 3, 2024

This PR is based on PR #10157 and requires it to be merged.

When merged this pull request will:

  • Adds a public function (addClassBlacklist) to blacklist ammo classes from producing fragments, spall, or both.
  • Changes the current blacklist function to be public and adds some param checks.

Feedback on the wording for the function descriptions is most welcome (and the code itself)!

IMPORTANT

  • If the contribution affects the documentation, please include your changes in this pull request so the documentation will appear on the website.
  • Development Guidelines are read, understood and applied.
  • Title of this PR uses our standard template Component - Add|Fix|Improve|Change|Make|Remove {changes}.

lambdatiger and others added 30 commits January 15, 2024 15:36
Co-authored-by: johnb432 <58661205+johnb432@users.noreply.github.com>
Co-authored-by: johnb432 <58661205+johnb432@users.noreply.github.com>
quick testing shows a whole 600ns increase in

Co-authored-by: johnb432 <58661205+johnb432@users.noreply.github.com>
…shooters

Co-authored-by: johnb432 <58661205+johnb432@users.noreply.github.com>
Co-authored-by: johnb432 <58661205+johnb432@users.noreply.github.com>
Comment on lines 5 to 6
* The function takes three arguments, although the last two are optional. The first argument is the ammo class name to ignore, while the second and third are
* whether the class should be blacklisted from producing fragments and spall, respectively. Once an ammo has been blacklisted, it can be removed from the blacklist
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be placed in the "arguments" section.
The two sentences here are useless imo, last sentence is useful info ('Once an...').

Copy link
Contributor Author

@lambdatiger lambdatiger Aug 14, 2024

Choose a reason for hiding this comment

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

I always felt confused and under explained when reading the ACE documentation, but that's probably a me thing. I'll move relevant information to the arguments/example and delete the rest. Let me know if that's still too much

@lambdatiger lambdatiger marked this pull request as ready for review September 1, 2024 22:57
@LinkIsGrim
Copy link
Contributor

LinkIsGrim commented Sep 18, 2024

Naming is a bit weird to me: add implies you can only add to the blacklist, not remove from it. set would be more appropriate I think

That being said, it's trivial. Anyone touching this is going to read the docs, and if a class is being added then removed from the blacklist, there's some bad decisions being made somewhere.

@lambdatiger
Copy link
Contributor Author

Naming is a bit weird to me: add implies you can only add to the blacklist, not remove from it. set would be more appropriate I think

That being said, it's trivial. Anyone touching this is going to read the docs, and if a class is being added then removed from the blacklist, there's some bad decisions being made somewhere.

Sure, would you say modifyClassBlacklist? Or do you have suggestions?

@LinkIsGrim
Copy link
Contributor

Naming is a bit weird to me: add implies you can only add to the blacklist, not remove from it. set would be more appropriate I think
That being said, it's trivial. Anyone touching this is going to read the docs, and if a class is being added then removed from the blacklist, there's some bad decisions being made somewhere.

Sure, would you say modifyClassBlacklist? Or do you have suggestions?

setClassBlacklisted makes the most sense to me, set indicating that it can both black/white-list while also indicating that we're not setting the blacklist itself, just the state of that class

@LinkIsGrim LinkIsGrim added the kind/enhancement Release Notes: **IMPROVED:** label Sep 18, 2024
@LinkIsGrim LinkIsGrim added this to the 3.18.0 milestone Sep 18, 2024
@LinkIsGrim LinkIsGrim merged commit 7cfdaa7 into acemod:master Sep 18, 2024
3 of 4 checks passed
@lambdatiger
Copy link
Contributor Author

Missed some semicolons on function return values

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Release Notes: **IMPROVED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants