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

Added an OnPvP compatability event #356

Conversation

VinyarionHyarmendacil
Copy link
Member

@VinyarionHyarmendacil VinyarionHyarmendacil commented Jul 29, 2019

See Issue #338.

Alleviate the Forge bug causing duplicate posts of the LivingAttackEvent. Forge posts this event twice, once in EntityPlayer, and once in EntityLivingBase, so this tells if an EntityPlayer got hurt but the event was posted as an EntityLivingBase, then immediately returns, so nothing important gets run twice.
Copy link
Contributor

@mahtaran mahtaran left a comment

Choose a reason for hiding this comment

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

Looks good overall, here is my code review.

* function will be invoked twice per attack - this is because of a Forge bug.
* Cancels combat events associated with PvP-disabled players. Note that
* this function will be invoked twice per attack - this is because of a
* Forge bug, but the {@link PvPCommonUtils isCurrentAttackDuplicate} call
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Forge bug, but the {@link PvPCommonUtils isCurrentAttackDuplicate} call
* Forge bug, but the {@link PvPCommonUtils#isCurrentAttackDuplicate()} call

Possibly the parentheses after the method are not required, not too sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

It works with them, so they might as well be added.

Copy link
Member

@CraftedMods CraftedMods left a comment

Choose a reason for hiding this comment

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

Nice! Just a few little things, but then I think we can merge it.

@CraftedMods CraftedMods added the compatibility Improve the interaction with other things label Jul 29, 2019
@CraftedMods CraftedMods added this to the 2.0.0-BETA.2 milestone Jul 29, 2019
@CraftedMods
Copy link
Member

@VinyarionHyarmendacil When @mahtaran does approve it too, please do a squash-merge (should be the only option) and use Closed #338 and closed #23 as commit message and PR #356 as commit description.

Copy link
Contributor

@mahtaran mahtaran left a comment

Choose a reason for hiding this comment

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

Almost (see comment for why I approved it.)

VinyarionHyarmendacil and others added 2 commits July 31, 2019 08:22
`pvpadmin resetCooldown <player>`
and
`pvpadmin spy <player> [on|off]`
@CraftedMods
Copy link
Member

@VinyarionHyarmendacil The pvpadmin change doesn't belong to this PR. PRs are centered around one feature. Please move that to another one.

@VinyarionHyarmendacil
Copy link
Member Author

Yeah, I accidently pushed it, didn't mean to.

@VinyarionHyarmendacil VinyarionHyarmendacil merged commit cccc19c into Lembas-Modding-Team:development_not-reviewed Jul 31, 2019
CraftedMods added a commit to CraftedMods/pvp-mode that referenced this pull request Aug 13, 2019
CraftedMods pushed a commit that referenced this pull request Aug 26, 2019
* Added an OnPvP compatability event

See Issue #338:
#338

* Fixed duplicate LivingAttackEvents

Alleviate the Forge bug causing duplicate posts of the
LivingAttackEvent. Forge posts this event twice, once in EntityPlayer,
and once in EntityLivingBase, so this tells if an EntityPlayer got hurt
but the event was posted as an EntityLivingBase, then immediately
returns, so nothing important gets run twice.

* Review 1: Added an OnPvP compatability event

* Improved pvpadmin command

`pvpadmin resetCooldown <player>`
and
`pvpadmin spy <player> [on|off]`

* Review 2: Added an OnPvP compatability event

Co-Authored-By: Mahtaran <[email protected]>

Update src/main/java/pvpmode/api/server/compatibility/events/OnPvPEvent.java

Co-Authored-By: Mahtaran <[email protected]>
CraftedMods pushed a commit that referenced this pull request Aug 26, 2019
* Added an OnPvP compatability event

See Issue #338:
#338

* Fixed duplicate LivingAttackEvents

Alleviate the Forge bug causing duplicate posts of the
LivingAttackEvent. Forge posts this event twice, once in EntityPlayer,
and once in EntityLivingBase, so this tells if an EntityPlayer got hurt
but the event was posted as an EntityLivingBase, then immediately
returns, so nothing important gets run twice.

* Review 1: Added an OnPvP compatability event

* Improved pvpadmin command

`pvpadmin resetCooldown <player>`
and
`pvpadmin spy <player> [on|off]`

* Review 2: Added an OnPvP compatability event

Co-Authored-By: Mahtaran <[email protected]>

Update src/main/java/pvpmode/api/server/compatibility/events/OnPvPEvent.java

Co-Authored-By: Mahtaran <[email protected]>
CraftedMods pushed a commit that referenced this pull request Aug 26, 2019
* Added an OnPvP compatability event

See Issue #338:
#338

* Fixed duplicate LivingAttackEvents

Alleviate the Forge bug causing duplicate posts of the
LivingAttackEvent. Forge posts this event twice, once in EntityPlayer,
and once in EntityLivingBase, so this tells if an EntityPlayer got hurt
but the event was posted as an EntityLivingBase, then immediately
returns, so nothing important gets run twice.

* Review 1: Added an OnPvP compatability event

* Improved pvpadmin command

`pvpadmin resetCooldown <player>`
and
`pvpadmin spy <player> [on|off]`

* Review 2: Added an OnPvP compatability event

Co-Authored-By: Mahtaran <[email protected]>

Update src/main/java/pvpmode/api/server/compatibility/events/OnPvPEvent.java

Co-Authored-By: Mahtaran <[email protected]>
CraftedMods added a commit to CraftedMods/pvp-mode that referenced this pull request Aug 26, 2019
CraftedMods added a commit that referenced this pull request Aug 26, 2019
VinyarionHyarmendacil added a commit that referenced this pull request Dec 16, 2019
* Renamed the faction placeholders HARAD and RHUN

Fixes a warning because the faction RHUN exists too.

* Added support for pledging specifiers

* Updated to v35.2 of the LOTR Mod

* Fixed a crash in singleplayer

* Added an OnPvP compatability event

See Issue #338: #338

* Fixed duplicate LivingAttackEvents

Alleviate the Forge bug causing duplicate posts of the LivingAttackEvent. Forge posts this event twice, once in EntityPlayer, and once in EntityLivingBase, so this tells if an EntityPlayer got hurt but the event was posted as an EntityLivingBase, then immediately returns, so nothing important gets run twice.

* Review 1: Added an OnPvP compatability event

* Improved pvpadmin command

`pvpadmin resetCooldown <player>`
and
`pvpadmin spy <player> [on|off]`

* Review 2: Added an OnPvP compatability event

Co-Authored-By: Mahtaran <[email protected]>

* Revert "Improved pvpadmin command"

This reverts commit f69c7c9.

* Added an OnPvP compatability event (#356)

* Added an OnPvP compatability event

See Issue #338: #338

* Fixed duplicate LivingAttackEvents

Alleviate the Forge bug causing duplicate posts of the LivingAttackEvent. Forge posts this event twice, once in EntityPlayer, and once in EntityLivingBase, so this tells if an EntityPlayer got hurt but the event was posted as an EntityLivingBase, then immediately returns, so nothing important gets run twice.

* Review 1: Added an OnPvP compatability event

* Improved pvpadmin command

`pvpadmin resetCooldown <player>`
and
`pvpadmin spy <player> [on|off]`

* Review 2: Added an OnPvP compatability event

Co-Authored-By: Mahtaran <[email protected]>

* Improved pvpadmin command, again

`pvpadmin resetCooldown <player>`
and
`pvpadmin spy <player> [on|off]`

* Added togglable ability to poison drinks

See Issue #333

* Revert "Added togglable ability to poison drinks"

This reverts commit 510ebdd.

* Update src/main/java/pvpmode/internal/server/command/PvPCommandAdmin.java

Co-Authored-By: Mahtaran <[email protected]>

* Update CHANGELOG.md

* Update ServerCommandConstants.java

* Fixed whitespace and incorrect conflict resolution
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility Improve the interaction with other things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants