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 pistons in subclaims #1056

Merged
merged 3 commits into from Oct 12, 2020
Merged

Fix pistons in subclaims #1056

merged 3 commits into from Oct 12, 2020

Conversation

FrankHeijden
Copy link

Pistons which are trying to push a block in pistons will not get fired:

2020-10-11_14 56 54

This PR fixes the issue.

@Jikoo
Copy link
Collaborator

Jikoo commented Oct 11, 2020

Oh gross, forgot that subclaims don't set owner ID. May be better to add a method Claim#getOwnerID to pair with Claim#getOwnerName, then use that here. There are several uses in pistons that probably need the fix, not just EVERYWHERE_SIMPLE.

@Jikoo
Copy link
Collaborator

Jikoo commented Oct 11, 2020

Also thank you for this, I know I missed that in my ClaimPermissionEvent PR too.

Copy link
Collaborator

@Jikoo Jikoo 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 to me 👍

@FrankHeijden FrankHeijden changed the title Fix pistons in subclaims not firing with PistonMode.EVERYWHERE_SIMPLE Fix pistons in subclaims Oct 11, 2020
@FrankHeijden
Copy link
Author

FrankHeijden commented Oct 11, 2020

I noticed more issues with the new Piston checks. This is best reproducible when there are many claims in the area; the boundary checks are off. Some parts of the claim allow pushing, some parts do not. I think the bounding box checks may be a bit overcomplicated regarding that matter. I think it's just best to get the claim(s) of the pushed block and the piston and compare these two, not doing any boundingbox checks.

@Jikoo
Copy link
Collaborator

Jikoo commented Oct 11, 2020

You're probably seeing the absence of #1036, would make sure you pull that patch onto whatever build you're testing with.

@Jikoo
Copy link
Collaborator

Jikoo commented Oct 11, 2020

Also yes, the bounding box is not perfect, it's just the maximum affected zone. If you look at the writeup for EVERYWHERE_SIMPLE you'll see an image I included highlighting the shortcomings - the claim does not interfere with the piston and blocks, but retraction is blocked because the bounding box conflicts. If you want exact mode, EVERYWHERE exists for that reason.

@FrankHeijden
Copy link
Author

Also yes, the bounding box is not perfect, it's just the maximum affected zone. If you look at the writeup for EVERYWHERE_SIMPLE you'll see an image I included highlighting the shortcomings - the claim does not interfere with the piston and blocks, but retraction is blocked because the bounding box conflicts. If you want exact mode, EVERYWHERE exists for that reason.

No I mean, when you have a claim solely in certain parts of your claim, it wont extend at all. not even on the boundary. I will be using the "EVERYWHERE" setting from now on.

@Jikoo
Copy link
Collaborator

Jikoo commented Oct 11, 2020

No I mean, when you have a claim solely in certain parts of your claim, it wont extend at all. not even on the boundary. I will be using the "EVERYWHERE" setting from now on.

Could you attach a screenshot of a setup like that, ideally with F3 up? If there is a problem I'm perfectly happy to dive into it myself, but I haven't been hearing about that in the limited live testing I've been running.

@FrankHeijden
Copy link
Author

Im not able to reproduce on my testserver unfortunately, in a hugely dense claiming area (e.g. a town) this behaviour pops up inconsistently though, some parts of a non-sub claim do work as intended, and some do not.

@RoboMWM
Copy link

RoboMWM commented Oct 12, 2020

ya, a getter is cool for Claim, I get it, Claim is bad, but at the same time I kinda don't want people to start using this just cuz it exists now when Claim may change again. Idk. whatever.

@RoboMWM RoboMWM merged commit af25b28 into GriefPrevention:master Oct 12, 2020
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.

None yet

3 participants