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 dispensers and bubble columns #18

Open
wants to merge 3 commits into
base: master
from

Conversation

@bermudalocket
Copy link
Member

commented Aug 23, 2019

Version 1.1.20.

Fixes two issues:

  1. A dispenser containing an empty bucket cannot pick up water from a waterlogged block.
  2. Bubble columns cannot be flowed / bubble columns do not form from a single flowed water source.

Also improves flow logic. See https://hub.spigotmc.org/jira/browse/SPIGOT-4759 for more information on why the jankiness is necessary.

@totemo

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

Is that removing all block metadata from a block?

Why not this?

EDIT: Ah just the SafeBuckets metadata... Why the difference though?

@bermudalocket

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2019

Is that removing all block metadata from a block?

Why not this?

EDIT: Ah just the SafeBuckets metadata... Why the difference though?

You're right: that should set the metadata to false, not remove it. I made the distinction between a block having no safety metadata and a block having metadata set to false so that once I added flow logging we would be able to tell the difference between a liquid flowing because of worldgen (i.e. null metadata) and a liquid flowing because it was flowed by staff or a player (false metadata).

I did not get a chance to add the logging so as of right now the two states are basically isomorphic, but it should probably be changed for clarity. Let me know if you would like me to make that change and I will push another commit.

Thanks!

@totemo

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

Right, now I understand why removeSafe() exists.

The specific thing I was picking up on was that in the former link you're removing all metadata associated with SafeBuckets, regardless of key, whereas in the latter you are removing only a specific key. So the former is a future bug if other SafeBuckets keys are added.

So if there's no compelling reason to remove all those keys (all one of them), let's get L130-132 as just:

BlockStoreApi.removeBlockMeta(block, PLUGIN, METADATA_KEY);

I think I'd be happy with just getting this fix through first. There are other problems in SafeBuckets that would be better to be fixed before trying to add new functionality.

Thanks.

@bermudalocket

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2019

Right, now I understand why removeSafe() exists.

The specific thing I was picking up on was that in the former link you're removing all metadata associated with SafeBuckets, regardless of key, whereas in the latter you are removing only a specific key. So the former is a future bug if other SafeBuckets keys are added.

So if there's no compelling reason to remove all those keys (all one of them), let's get L130-132 as just:

BlockStoreApi.removeBlockMeta(block, PLUGIN, METADATA_KEY);

I think I'd be happy with just getting this fix through first. There are other problems in SafeBuckets that would be better to be fixed before trying to add new functionality.

Thanks.

Sure thing, I'll get that pushed shortly.

Just to clarify re: logging, I was speaking in the past perfect tense: it was a feature I planned approximately a year ago now that never fully came to fruition. I wasn't trying to stage for it here. To be honest I don't quite remember why I opted to remove all the block metadata instead of just setting it to false, which makes more sense here. Regardless, you're right: it's a future bug waiting to happen.

And re: the other bugs, could you open issues for those so I can help fix them? If you don't have the time to open issues for each, feel free to send me a list and I will open them.

Last but not least, thanks for taking the time to review this pull req. Let me know if there's anything I can do differently moving forward to help facilitate the review process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.