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.

Sign up for GitHub

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

Jump to bottom

Allow preventing critical hits #12319

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allow preventing critical hits #12319

wants to merge 1 commit into from
+88 −8

Conversation

dawon
Copy link
Contributor

@dawon dawon commented Mar 19, 2025

See #12318

@dawon dawon requested a review from a team as a code owner March 19, 2025 14:30
@Doc94
Copy link
Contributor

Doc94 commented Mar 19, 2025

Hi, hmm currently by the event you can only prevent the hit but cannot know if already was "prevented" by the config not?

@dawon
Copy link
Contributor Author

dawon commented Mar 19, 2025

Oh I could default the value based on the config option, good idea!

@Owen1212055
Copy link
Member

I would rather have this as a separate event imo. Tying this to this event means that if this logic moves it'll make this setter painful to upkeep.

@Leguan16
Copy link
Contributor

I think I unfortunately have to agree with Owen here.

@dawon dawon force-pushed the feature/allow-preventing-critical-hits branch 2 times, most recently from f3ab6ff to b989cff Compare March 19, 2025 22:10
@dawon
Copy link
Contributor Author

dawon commented Mar 19, 2025

I've updated the PR so now there is a separate event, that you can use to cancel or enforce a critical hit as well as change the crit modifier...

@dawon dawon force-pushed the feature/allow-preventing-critical-hits branch from b989cff to a9751c9 Compare March 20, 2025 06:34
@notTamion
Copy link
Contributor

i think a better name would be something like PlayerAttackDamageEvent considering this event is called for all hits and not just critical ones

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Doc94 Doc94 Doc94 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
Paper PR Queue
Status: Awaiting review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dawon @Doc94 @Owen1212055 @Leguan16 @notTamion