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

First pass at implementing the /clone command #590

Merged
merged 4 commits into from Dec 21, 2017

Conversation

Projects
None yet
3 participants
@smartboyathome
Contributor

smartboyathome commented Nov 26, 2017

So, this is a big one that requires a lot of manual testing (tried writing some unit tests, but the mocks just weren't that easy to create). This implementation shouldn't require too much memory when executing, due to the dynamic direction of the iteration in order to avoid any overlap. By making it dynamic for each axis, I was able to clone without staging copies of each block in memory first. As mentioned, I did as much manual testing as I could, but doubtless I have missed several scenarios that may break this. More eyes are needed on this change before it should be merged.

if (current >= 0) {
// Could use a post-decrement operator here, but this is a bit more clear in getting the meaning across.
int retval = current;
current -= 1;

This comment has been minimized.

@momothereal

momothereal Nov 26, 2017

Member

I understand why you'd prefer not using int retval = current--;, but maybe replace the current -= 1 line with current--;

This comment has been minimized.

@smartboyathome

smartboyathome Nov 26, 2017

Contributor

Yeah, my biggest problem with the post- and pre- increment/decrement operators is that, in variable assignments, its not obvious which returns the old value and which returns the new value. I can definitely change that line to be current-- though.

}
if (!CommandUtils.isPhysical(sender)) {
sender.sendMessage("Cannot access blocks outside of the world");

This comment has been minimized.

@momothereal

momothereal Nov 26, 2017

Member

I think the error message and the if-statement don't match here. (e.g. should be This command may only be executed by physical objects or something. I think other commands have this message somewhere.

} catch (NumberFormatException ignored) {
}
if (data == null || 0 > data || data > 15) {
sender.sendMessage(ChatColor.RED + "Usage: " + usageMessage);

This comment has been minimized.

@momothereal

momothereal Nov 26, 2017

Member

Give a more precise error here (e.g. Invalid data: <data>)

break;
default:
sender.sendMessage(ChatColor.RED + "Usage: " + usageMessage);

This comment has been minimized.

@momothereal

momothereal Nov 26, 2017

Member

Also here, say something like Unknown block filter: <filter>

"Clones a section of the world.", "/clone <x1> <y1> <z1> <x2> <y2> <z2> <x> <y> <z> [maskMode] [cloneMode] [tileName] [tileData]",
Collections.emptyList()
);
setPermission("minecraft.command.seed");

This comment has been minimized.

@momothereal

momothereal Nov 30, 2017

Member

Wrong permission :^)

}
}
sender.sendMessage("Cloned " + blocksCloned + " blocks.");

This comment has been minimized.

@momothereal

momothereal Nov 30, 2017

Member

If blocksCloned ends up being 0, should another message pop up here? (e.g. No blocks were cloned)

@mastercoms mastercoms merged commit 35fff3e into GlowstoneMC:dev Dec 21, 2017

2 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details

@momothereal momothereal referenced this pull request Dec 30, 2017

Open

Implementing vanilla commands #499

40 of 53 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment