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

Fixing Handling of Performers and Cleaning Some Code #342

Open
wants to merge 4 commits into
base: sponge-1.12
Choose a base branch
from

Conversation

nlipiarski
Copy link

In this pull request, I updated how Voxel Sniper was handling performance modifiers on brushes to properly support the material, ink, and combo performers for both the placing and replacing of blocks. With this change, I updated the vi and vir change to take multiple block traits as arguments and ironed out some of the code in v and vr to better handle BlockStates and be more idiomatic. In addition, I cleaned up some of the naming conventions in SnipeData to be more consistent. The majority of the files affected by this pull request stem from that.

@Deamon5550 Deamon5550 self-requested a review October 15, 2019 18:07
Copy link
Member

@gabizou gabizou left a comment

Choose a reason for hiding this comment

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

While I appreciate some tender loving care being applied to VS, it's a lot of changes that are cluttered from extra noise compared to actually fixing the handling of performers. I do appreciate the effort put in to the PR, just hoping that the feedback will improve the PR from a maintenance perspective.

src/main/java/com/thevoxelbox/voxelsniper/Message.java Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
src/main/java/com/thevoxelbox/voxelsniper/SnipeData.java Outdated Show resolved Hide resolved
src/main/java/com/thevoxelbox/voxelsniper/SnipeData.java Outdated Show resolved Hide resolved
// new SniperMaterialChangedEvent(this, toolId, new MaterialData(originalVoxel, snipeData.getData()),
// new MaterialData(snipeData.getVoxelId(), snipeData.getData()));
// Bukkit.getPluginManager().callEvent(event);
snipeData.setVoxelState(targetBlock.getBlockType().getDefaultState());
Copy link
Member

Choose a reason for hiding this comment

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

If you're really wanting to fix it, wouldn't it be best to use the target block's block state and not the default state of the block type?

Copy link
Author

Choose a reason for hiding this comment

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

I followed what was already there in the code so if you are holding the arrow it sets it to be the default state and if you are holding gunpowder it takes the whole block state. See line 154.

@nlipiarski
Copy link
Author

nlipiarski commented Oct 18, 2019

OK, I split out the some changes into #343 and #344. I though I could stack the PRs, but I guess not so this one still includes those changes but once they are merged it should be fine.

Copy link
Member

@Deamon5550 Deamon5550 left a comment

Choose a reason for hiding this comment

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

Could you rebase this now that your other two PRs have been merged.

COMBO,
NONE;

public String toString() {
switch (this) {
Copy link
Member

Choose a reason for hiding this comment

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

This should just be a field on the enum.

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean by that? The toString method is already part of the enum class.

@nlipiarski
Copy link
Author

I rebased my branch onto the new changes, but there was some problem with line endings in PerformBrush.java that ended up making it look like the entire file was changed.

@nlipiarski
Copy link
Author

@Deamon5550 @gabizou ping on this?

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.

3 participants