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

Fix Nether portals not disappearing when frame destroyed #28

Merged
merged 1 commit into from
Mar 7, 2015

Conversation

literalplus
Copy link
Contributor

Hello,
The issue:
In the current build of this plugin, players are not able to destroy nether portals since when they destroy any part of the Obsidian frame, the Portal blocks don't vanish. (Vanilla/Spigot behaviour: Portal disappears when Obsidian destroyed) However, admins may wish to run this plugin on a server which allows normal players to destroy native Nether portals.

PR Breakdown:
This PR adds an extra check to the BlockPhysicsEvent handler which makes sure that we only cancel physics events actually relevant to WarpPortals. This PR blocks the following cases only:

  • A portal is created using a gold block template and replacing said template with portal blocks causes them to update (If this is not prohibited, only two blocks of the portal will appear while the others will silently turn into air upon creation - I'll guess that this is not the intended behaviour)
  • A block inside a registered WarpPortal receives a physics update (This could for example rotate portal blocks or destroy them entirely if not surrounded by an appropriate frame [citation needed])

Potential problems:
This PR does take a blacklist approach to fix mentioned issue as opposed to my previous attempt (#26), which took a whitelist approach (It attempted to only allow portal block physics for intended Vanilla behaviour - I tried to expand this as far as to allow the whole portal to be destroyed when removing a single block, but the current Bukkit API (BlockPhysicsEvent) does not provide a clean way to accomplish this). This might cause issues with physics updates we want to block for WarpPortals that I couldn't think of.

Testing materials:
I have tested that this change stops the issue mentioned above from happening (restoring full Vanilla behaviour, unlike #26). I have also verified that it does not interfere with the portal creation process as outlined in README.md for portals with nether portal block body. (I haven't tested any other bodies and I don't know of any other ways to create a WarpPortal) This PR has been tested in a clean environment. I can provide the artifact I used to test, if desired.
This is a picture of my test setup for proof and to improve this PR description's layout:
not relevant actually
The three portals to the left are actual WarpPortals which I tested to work correctly, even when manually changing some blocks inside or upon reconnecting. The rightmost portal is an actual Vanilla Nether Portal, which I tested to work as it would without this plugin installed.

Additional remarks:
This is a reimplementation of #26 with all known problems fixed. I initially tried to keep #26, but GitHub wouldn't allow me to reopen it with force-pushed commits.
Also, developing features and bugfixes for this project would be far easier if it would use a central build system, such as Apache Maven and/or if the compilation process were outlined somewhere. I, for example, could not locate the required com.mccraftaholics.warpportalscommon package, which is used by both classes in com.mccraftaholics.warpportals.remote. (I temporarily removed those classes for my testing purposes)
Frthermore, I'd like to apologize for the unnecessary whitespace diffs introduced into my patch. (my IDE wouldn't let me omit those) If desired, I take steps to remove those diffs from the patch.

Thanks!

TL;DR: Fixes Vanilla Nether Portals not disappearing when the frame is destroyed.

@awendland awendland merged commit 9a91c1d into McCraftaholics:master Mar 7, 2015
@awendland
Copy link
Contributor

Sorry for the extensive hiatus. I have finally merged this PR! Also, WarpPortals has been migrated to the maven build system.

awendland added a commit that referenced this pull request Mar 7, 2015
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