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

Allow fluids to flow between claims of the same owner #1218

Merged
merged 5 commits into from
Jun 19, 2023

Conversation

FreeMonoid
Copy link

Context: https://korobi.vq.lc/network/spigot/channel/griefprevention/logs/2020/11/19/#L14

00:13:32 Dummycord <F​re​eM​on​oi​d> I was thinking it might be worth checking for claim owner instead of just whether it's the same claim, in case it's this uncommon case of same owner having two adjacent claims
00:26:19 RoboMWM hm
00:26:21 RoboMWM I c
00:26:44 RoboMWM should just get rid of it
00:26:55 RoboMWM pretty sure bigscary prolly removed only part of it due to performance issues
00:27:00 RoboMWM and didn't get it all
00:29:58 Dummycord <F​re​eM​on​oi​d> get rid of preventing flow in survival? idk, it's pretty easy way to grief someone with water or worse, lava (I see it everywhere on servers with WG claims cause it's super easy to abuse there) so I think it's pretty useful
00:30:44 RoboMWM wild -> claim isn't blocked is i
00:30:46 RoboMWM it
00:31:01 Dummycord <F​re​eM​on​oi​d> it is blocked
00:31:12 Dummycord <F​re​eM​on​oi​d> you can still mess up wilderness, but at least claims are intact
00:31:17 RoboMWM huh
00:31:25 RoboMWM I thought he claimed to remove that
00:31:33 Dummycord <F​re​eM​on​oi​d> ¯\_(ツ)_/¯
00:32:09 RoboMWM there's a lot of ways you could mess up someone, and usually at that point I advocate to make ur borders stronger/bigger
00:32:20 RoboMWM but eh
00:32:33 RoboMWM only thing that ppl complained about are pistons
00:32:38 RoboMWM so I guess it stays
00:33:12 RoboMWM can PR the same owner check if u wanna - just be aware of admin claims having null owner and that sort of stuff

Tested combinations:

combinations

Each 10x10 square is a claim, color denotes the owner: green is wilderness, red is Player 1, yellow is Player 2, black is admin claim, white squares inside claims are subdivisions; sea lantern is where the water source block is.

@Hazugg

This comment has been minimized.

@smplfy
Copy link

smplfy commented May 4, 2021

This would be super helpful. Please merge if possible.

@Allowsik
Copy link

Allowsik commented Jun 4, 2022

This looks great i don't get it why they didn't review it yet.

@FreeMonoid
Copy link
Author

@RoboMWM (or maybe @Jikoo) can I ask you for a review of this older but still applicable PR, so it maybe can land in the next release?

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.

I think my one reservation is stuff like shop or display areas - someone filling their allotted subclaim with lava could grief adjacent decorations, or vice versa with restricted subclaims. Some of that is down to intelligent design for claim owners, but if we're introducing a new behavior, they can't really be expected to have planned around it. I guess GP could leave this to an addon, but it makes me a bit uncomfortable.

@FreeMonoid
Copy link
Author

@Jikoo sorry for a late reply: I've added a check for subdivisions, it now behaves exactly as 16.18.1 with regard to them:
2023-03-30_20 01 57
2023-03-30_20 41 35

Does this address your concerns?

@Jikoo
Copy link
Collaborator

Jikoo commented Mar 31, 2023

I also think a restricted subclaim should block flow from its parent because the list of people with build trust inside it has no relation to the list of people who have build trust in the claim as a whole. Otherwise yes, looks good!

and refactor the logic for checking whether fluid flow is allowed between two claims
@FreeMonoid
Copy link
Author

@Jikoo started adding a check for the restricted subclaims and the code became a bit too hard to follow, so I've ended up refactoring it a bit.

Re-did the test with subclaims added to the mix:
fluid flow test
Similar to initial test: green is wilderness, red is Player 1, yellow is Player 2, black is admin claim, white squares inside claims are regular subdivisions; pink squares inside claims are restricted subdivisions; sea lantern is where the water source block is.

Let me know if have any feedback on this!

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 excellent to me 👍 Sorry for asking for such a logical mess

@FreeMonoid
Copy link
Author

No problem at all! I think the implementation we ended up with here is much cleaner than what I initially proposed!

@RoboMWM RoboMWM added this to the 16.18.2 milestone Apr 9, 2023
@RoboMWM RoboMWM merged commit 64b348a into GriefPrevention:master Jun 19, 2023
1 check passed
@FreeMonoid FreeMonoid deleted the fluid-owner-check branch June 27, 2023 16:13
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

6 participants