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

[1.19.x] Enable placing fluids into waterlogged blocks with FluidUtil::tryPlaceFluid #9519

Merged

Conversation

cech12
Copy link
Contributor

@cech12 cech12 commented Jun 3, 2023

This PR adds the possibility to place fluids into waterlogged blocks with the helper method FluidUtil::tryPlaceFluid.
It enhances the consistency to the bugfix MC-127110 (You can't empty water buckets into waterlogged blocks) of Minecraft 1.19.3.

I added a simple fluid container test mod, which adds a fluid container which uses the FluidUtil::tryPlaceFluid and FluidUtil::tryPickUpFluid methods to interact with the world.

Right clicking a waterlogged block (waterlogged Slab or something else) will empty a filled fluid container by not placing any other water source (like the vanilla bucket).

Without the fix of the BlockWrapper class, the placement of the fluid into the waterlogged block fails and it tries to place the fluid at the block of the hit side. Failing on a waterlogged block is not consistent to vanilla.

…eFluid. Enhance consistency to 1.19.3 fix MC-127110 (You can't empty water buckets into waterlogged blocks)
@autoforge autoforge bot added Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. 1.19 labels Jun 3, 2023
@autoforge autoforge bot requested a review from a team June 3, 2023 19:37
@ChampionAsh5357 ChampionAsh5357 added Feature This request implements a new feature. Fluids This request touches or otherwise deals with the fluids system. These requests require special care. labels Jun 3, 2023
@cech12 cech12 changed the title [1.19.4] Enable placing fluids into waterlogged blocks with FluidUtil::tryPlaceFluid [1.19.x] Enable placing fluids into waterlogged blocks with FluidUtil::tryPlaceFluid Jun 3, 2023
Comment on lines -76 to +78
//If we are executing try to actually fill the container, if it failed return that we failed
if (action.simulate() || liquidContainer.placeLiquid(world, blockPos, state, resource.getFluid().getFluidType().getStateForPlacement(world, blockPos, resource)))
if (action.execute())
{
return FluidType.BUCKET_VOLUME;
liquidContainer.placeLiquid(world, blockPos, state, resource.getFluid().getFluidType().getStateForPlacement(world, blockPos, resource));
}
return FluidType.BUCKET_VOLUME;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this need to be changed? I don't understand what this is doing differently other than saying that the value should always be the bucket volume when it asks whether it could place the liquid, which doesn't make sense if it can't actually be placed.

Copy link
Contributor Author

@cech12 cech12 Jun 3, 2023

Choose a reason for hiding this comment

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

The liquidContainer is an implementation of the vanilla interface LiquidBlockContainer, which is implemeted by waterloggable blocks. If such a block is waterlogged the placeLiquid method returns false, because no liquid is added to this block. But when you look into the BucketItem implementation, how LiquidBlockContainer is handled there (since the bugfix MC-127110), the result of the placeLiquid method is ignored:

         } else if (block instanceof LiquidBlockContainer && ((LiquidBlockContainer)block).canPlaceLiquid(p_150717_,p_150718_,blockstate,content)) {
            ((LiquidBlockContainer)block).placeLiquid(p_150717_, p_150718_, blockstate, ((FlowingFluid)this.content).getSource(false));
            this.playEmptySound(p_150716_, p_150717_, p_150718_);
            return true;
         } else {

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I understand the reasoning now because of a quirk of vanilla. I'll double check the solution and test when I awake in half a day since I currently don't have the headspace to properly review this. However, at a general glance, the solution seems fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, I don't have an issue with the solution. The main issue is that it is a breaking change in logic, regardless of expected behavior, so I'm not sure whether this should be merged into 1.19 or just held off until 1.20 in roughly 24-36 hours.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice to hear that! :) I don't know, who can answer your question.^^ I can understand, that a breaking change shouldn't be merged into 1.19, but in my opinion it is a behaviour that should be there since vanilla 1.19.3. But how you decide is entirely up to you, :)

@cech12
Copy link
Contributor Author

cech12 commented Jul 8, 2023

@ChampionAsh5357
The 1.20 version of this PR was merged. Should this PR be closed, because it is a breaking change that isn't merged into 1.19?
Or is there a chance to merge it?

@LexManos LexManos merged commit b39e300 into MinecraftForge:1.19.x Jul 11, 2023
22 checks passed
@cech12 cech12 deleted the feature/1.19.4-waterloggable-blocks-bug branch July 11, 2023 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.19 Feature This request implements a new feature. Fluids This request touches or otherwise deals with the fluids system. These requests require special care. Triage This request requires the active attention of the Triage Team. Requires labelling or reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants