8000 EnderChest support 👻 by applenick · Pull Request #1045 · PGMDev/PGM · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

EnderChest support 👻 #1045

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
Merged
merged 4 commits into from
Oct 21, 2022
Merged

EnderChest support 👻 #1045

merged 4 commits into from
Oct 21, 2022

Conversation

applenick
Copy link
Member
@applenick applenick commented Aug 20, 2022 •
edited
Loading

EnderChest support

Hooray! 👻 This PR adds proper EnderChest support to PGM.

XML Example

<!-- Add drop-off locations for when player switches teams or leave the match -->
<enderchest>
  <!-- Each drop-off requires a region & filter attribute -->
  <dropoff region="red-spawn" filter="red-only"/>
  <dropoff region="blue-spawn" filter="blue-only"/>
</enderchest>

<!-- Specify a fallback location option for when no drop-off locations are defined (AUTO, KEEP, or DELETE) -->
<enderchest fallback="DELETE"/>

If you've got any feedback let me know and I'd be more than happy to make accommodations. As always these changes have been tested and should work as intended 👍

Signed-off-by: applenick applenick@users.noreply.github.com

@applenick applenick added the feature New feature or request label Aug 20, 2022
@applenick applenick requested a review from Electroid as a code owner August 20, 2022 07:01
@applenick applenick requested a review from Pablete1234 August 20, 2022 07:04
@KingOfSquares
Copy link
Contributor
KingOfSquares commented Aug 20, 2022 •
edited
Loading

I believe players still would be blocked from placing ender chests because of the listener in EventFilterMatchModule

PGM/core/src/main/java/tc/oc/pgm/modules/EventFilterMatchModule.java

Lines 259 to 271 in 963f02a

@EventHandler(priority = EventPriority.LOWEST, ignoreCancelled = true)
public void onPlayerBlockChange(final PlayerBlockTransformEvent event) {
cancelUnlessInteracting(event, event.getPlayerState());
if (!event.isCancelled() && event.getNewState().getType() == Material.ENDER_CHEST) {
cancel(
event,
true,
event.getWorld(),
event.getPlayer(),
translatable("match.disabled.enderChest"));
}
}

Maybe also filter that on the enabled boolean?

EDIT: Or maybe not? Since you are cancelling the event, but then this listener is redundant

Also, with this addition maybe it would also make sense to add ender chest support to kits?

@applenick
Copy link
Member Author
applenick commented Aug 20, 2022 •
edited
Loading

@KingOfSquares
Great point! I believe you're correct, my changes don't currently adjust EventFilterMatchModule blocking the placing/breaking of EnderChests. Personally I don't foresee many instances where map authors would want players to be able to freely place them, so chose to preserve that function while allowing interacting with existing EnderChests to be supported. Since the more common usage will be interacting with strategically placed chests on a map.

Would you suggest allowing them to be placed?

Also, with this addition maybe it would also make sense to add ender chest support to kits?

Could you elaborate on this?

Thanks!

@KingOfSquares
Copy link
Contributor

@KingOfSquares Great point! I believe you're correct, my changes don't currently adjust EventFilterMatchModule blocking the placing/breaking of EnderChests. Personally I don't foresee many instances where map authors would want players to be able to freely place them, so chose to preserve that function while allowing interacting with existing EnderChests to be supported. Since the more common usage will be interacting with strategically placed chests on a map.

Would you suggest allowing them to be placed?

Also, with this addition maybe it would also make sense to add ender chest support to kits?

Could you elaborate on this?

Thanks!

For the kit thing, there is some support already in Slot.java for being able to place items in players ender chests through kits. A kit could potentially have an <item slot="enderchest.1" material="stone sword"/>

For placing ender chests: I agree that the ability to being able to place ender chests is not correct in most situations, but since they are now usable I only think its a matter of time before someone requests how to enable placing them(for some reason). Why not default it to false and include the possibility as an attribute? I also believe the error message is a little misleading, Ender chests are disabled should probably be something like Placing ender chests are disabled

@TheRealPear TheRealPear mentioned this pull request Aug 27, 2022
Closed
18 tasks
Pablete1234
Pablete1234 requested changes Sep 1, 2022
View reviewed changes
@Electroid
Copy link
Member

Hey @applenick, thanks for working on this.

I think it would be better to integrate ender chest support into the existing Kit and Slot system. Also, maybe we already have support??

PGM/core/src/main/java/tc/oc/pgm/kits/Slot.java

Line 169 in a3992e8

public static class EnderChest extends Slot {

@applenick
Copy link
Member Author

Hey @applenick, thanks for working on this.

I think it would be better to integrate ender chest support into the existing Kit and Slot system. Also, maybe we already have support??

PGM/core/src/main/java/tc/oc/pgm/kits/Slot.java

Line 169 in a3992e8

public static class EnderChest extends Slot {

No problem! Excellent point about the kit/slot support, I believe Simon mentioned that too in earlier comments. Perhaps we should just scrap the custom inventory logic and have it rely on the built-in enderchests. That way they'll already support slots like mentioned. Though I believe it's important to keep some of the new functionality added like drop-off locations and the fallback attribute.

Will look into fixing this up over the weekend 👍

@Electroid Electroid force-pushed the dev branch 2 times, most recently from 20c50fe to d978236 Compare October 8, 2022 23:40
@applenick applenick requested a review from Pablete1234 October 12, 2022 18:00
@applenick
Copy link
Member Author
applenick commented Oct 12, 2022 •
edited
Loading

Finally had some time and fixed this up 🎉 Switched away from using the custom enderchest implementation we originally had, relying now on the traditional enderchest contents. We do lose the capability to customize enderchest row count, but I believe it's a fair tradeoff for being able to have enderchests remain compatible with the Kit/Slot system.

If there's any additional feedback let me know and I'll be happy to make adjustments 👍

@applenick
Add support for customizable EnderChests
466645e 
Signed-off-by: applenick <applenick@users.noreply.github.com>
@applenick
Replace persistent team chests with dropoffs
da0cb4e 
Signed-off-by: applenick <applenick@users.noreply.github.com>
@applenick
Swap custom enderchests implementation for vanilla
b7ffdb4 
Signed-off-by: applenick <applenick@users.noreply.github.com>
Pablete1234
Pablete1234 requested changes Oct 19, 2022
View reviewed changes
@applenick
Use new XML property parser with random validation
1ec8dde 
Signed-off-by: applenick <applenick@users.noreply.github.com>
@applenick applenick requested a review from Pablete1234 October 19, 2022 22:03
Electroid
Electroid approved these changes Oct 21, 2022
View reviewed changes
Copy link
Member
@Electroid Electroid left a comment

Choose a reason for hiding this comment

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

🚀

@Electroid Electroid merged commit 47eafaa into PGMDev:dev Oct 21, 2022
@TheRealPear TheRealPear mentioned this pull request Oct 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Electroid Electroid Electroid approved these changes

@Pablete1234 Pablete1234 Pablete1234 approved these changes

Assignees
No one assigned
Labels
feature New feature or request
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@applenick @KingOfSquares @Electroid @Pablete1234
0