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

Implement block states for commands and /testforblock command #579

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
3 participants
@momothereal
Member

momothereal commented Oct 22, 2017

This PR adds the system to parse and compare block state data as strings instead of numerical data values. This feature was added to some commands in 1.11, and allows using color=orange instead of the data value 1 for wool blocks, for example.

  • Implements a parsing system to read such block state strings and compare them
    • Currently only supports Wool block state as proof-of-concept, needs to be expanded to other blocks.
  • Implements the /testforblock command, which uses this new functionality.
    • Sample usage: /testforblock ~ ~-1 ~ minecraft:wool color=light_blue
  • Fixed an inconsistency with tilde notation (e.g. ~ ~-1 ~), where the relative Y value would be shifted by 0.5 blocks like the X and Z axis. Some tests needed to be fixed to pass with this correction.
  • Moved the block entity states inside the net.glowstone.block.entity.state package to avoid confusion.

I still need to add some documentation for the block state parsing system.

Related to #499

@momothereal momothereal added this to Review in 2017Q4 Oct 22, 2017

@smartboyathome

There you go, that's my feedback.

Show outdated Hide outdated src/main/java/net/glowstone/block/state/impl/WoolStateDataReader.java Outdated
public abstract T read(Material material, BlockStateData data) throws InvalidBlockStateException;
public abstract boolean matches(BlockStateData state, T data) throws InvalidBlockStateException;

This comment has been minimized.

@smartboyathome

smartboyathome Oct 26, 2017

Contributor

I feel like sticking this on the reader is a bit disjoint with how I implemented the Entity's tag matching. I'm not opposed to sticking it on the reader, but and if you want to keep it like this you should think about changing how the CompoundTag's matching works. Otherwise, I'd say you should implement the match method on the BlockStateData, similar to how CompoundTag implements its match method.

@smartboyathome

smartboyathome Oct 26, 2017

Contributor

I feel like sticking this on the reader is a bit disjoint with how I implemented the Entity's tag matching. I'm not opposed to sticking it on the reader, but and if you want to keep it like this you should think about changing how the CompoundTag's matching works. Otherwise, I'd say you should implement the match method on the BlockStateData, similar to how CompoundTag implements its match method.

Show outdated Hide outdated src/main/java/net/glowstone/block/state/BlockStateData.java Outdated
throw new InvalidBlockStateException(material, state);
}
BlockStateData data = new BlockStateData();
String[] split = state.trim().split(",");

This comment has been minimized.

@smartboyathome

smartboyathome Oct 26, 2017

Contributor

This is a case where I'd say a small Regex could make the expected syntax a bit clearer, and reduce the number of cases you need to check to throw InvalidBlockStateException. Looks like the regex you want is "^\\s*((\\w+)\\s*=\\s*(\\w+))\\s*(,\\s*(\\w+)\\s*=\\s*(\\w+)\\s*)*$". Looks intimidating, yes, but that is just keeping compatibility with the trimmed code above.

To use this regex, you'd be iterating through all the groups (starting at index 1 because the group at index 0 always matches the entire string), but taking only the second and third group out of every three, as the first will represent the entire "key=value" match.

@smartboyathome

smartboyathome Oct 26, 2017

Contributor

This is a case where I'd say a small Regex could make the expected syntax a bit clearer, and reduce the number of cases you need to check to throw InvalidBlockStateException. Looks like the regex you want is "^\\s*((\\w+)\\s*=\\s*(\\w+))\\s*(,\\s*(\\w+)\\s*=\\s*(\\w+)\\s*)*$". Looks intimidating, yes, but that is just keeping compatibility with the trimmed code above.

To use this regex, you'd be iterating through all the groups (starting at index 1 because the group at index 0 always matches the entire string), but taking only the second and third group out of every three, as the first will represent the entire "key=value" match.

This comment has been minimized.

@momothereal

momothereal Oct 26, 2017

Member

I hadn't thought of using a regex here, I will consider it. My only concern would be readability, but that can be resolved with a comment.

@momothereal

momothereal Oct 26, 2017

Member

I hadn't thought of using a regex here, I will consider it. My only concern would be readability, but that can be resolved with a comment.

This comment has been minimized.

@momothereal

momothereal Oct 27, 2017

Member

After checking, the regular expression you provided doesn't work all the time. For the string a=b,c=d,e=f:

Full match a=b,c=d,e=f
Group 1 a=b
Group 2 a
Group 3 b
Group 4 ,e=f
Group 5 e
Group 6 f

This would skip the middle section.

@momothereal

momothereal Oct 27, 2017

Member

After checking, the regular expression you provided doesn't work all the time. For the string a=b,c=d,e=f:

Full match a=b,c=d,e=f
Group 1 a=b
Group 2 a
Group 3 b
Group 4 ,e=f
Group 5 e
Group 6 f

This would skip the middle section.

return false;
}
String itemName = args[3];
if (!itemName.startsWith("minecraft:")) {

This comment has been minimized.

@smartboyathome

smartboyathome Oct 26, 2017

Contributor

I'm not as familiar with the plugin side of things, but if a plugin introduces a new block, will it still start with "minecraft:"? If not, then this check would seem to fail when trying to test for a "foo-plugin:bar" block.

@smartboyathome

smartboyathome Oct 26, 2017

Contributor

I'm not as familiar with the plugin side of things, but if a plugin introduces a new block, will it still start with "minecraft:"? If not, then this check would seem to fail when trying to test for a "foo-plugin:bar" block.

This comment has been minimized.

@momothereal

momothereal Oct 26, 2017

Member

You can't register new blocks/items with Bukkit. The ids are registered in the ItemIds class and all start with the minecraft namespace. Adding this functionality is outside of the scope of the PR.

@momothereal

momothereal Oct 26, 2017

Member

You can't register new blocks/items with Bukkit. The ids are registered in the ItemIds class and all start with the minecraft namespace. Adding this functionality is outside of the scope of the PR.

return false;
}
if (args.length > 4) {
String state = args[4];

This comment has been minimized.

@smartboyathome

smartboyathome Oct 26, 2017

Contributor

Since we know this same "check the data value or state argument" is used in at least three commands (clone and testforblock, and testforblocks), mind abstracting this check out into a CommandUtils method? May want to return a custom type with two properties, Integer data and BlockStateData stateData, for ease of use in these commands.

@smartboyathome

smartboyathome Oct 26, 2017

Contributor

Since we know this same "check the data value or state argument" is used in at least three commands (clone and testforblock, and testforblocks), mind abstracting this check out into a CommandUtils method? May want to return a custom type with two properties, Integer data and BlockStateData stateData, for ease of use in these commands.

This comment has been minimized.

@momothereal

momothereal Oct 26, 2017

Member

Will do.

@momothereal

momothereal Oct 26, 2017

Member

Will do.

}
}
if (args.length > 5 && block.getBlockEntity() != null) {
String dataTag = String.join(" ", new ArrayList<>(Arrays.asList(args)).subList(5, args.length));

This comment has been minimized.

@smartboyathome

smartboyathome Oct 26, 2017

Contributor

Given we do this CompoundTag check in a few commands as well now, we might want to think about also abstracting this out into a CommandUtils method.

@smartboyathome

smartboyathome Oct 26, 2017

Contributor

Given we do this CompoundTag check in a few commands as well now, we might want to think about also abstracting this out into a CommandUtils method.

This comment has been minimized.

@momothereal

momothereal Oct 26, 2017

Member

Good point, but it's outside of the scope of the PR. I'll consider it once this is merged.

@momothereal

momothereal Oct 26, 2017

Member

Good point, but it's outside of the scope of the PR. I'll consider it once this is merged.

Fix some things based on feedback.
* Fix exception when parsing a state with no value (something=)
* Move some common state utils to CommandUtils
* Boxed map instead of extending it (BlockStateData)

@momothereal momothereal requested a review from mastercoms Oct 28, 2017

@momothereal

This comment has been minimized.

Show comment
Hide comment
@momothereal

momothereal Oct 30, 2017

Member

I need approval to merge this for the release of 2017.10 on Wednesday. I can address the documentation and further design changes later on.

Member

momothereal commented Oct 30, 2017

I need approval to merge this for the release of 2017.10 on Wednesday. I can address the documentation and further design changes later on.

@momothereal

This comment has been minimized.

Show comment
Hide comment
@momothereal

momothereal Oct 31, 2017

Member

Merged in 18a3b49.

Member

momothereal commented Oct 31, 2017

Merged in 18a3b49.

@momothereal momothereal deleted the states branch Oct 31, 2017

@momothereal momothereal moved this from Review to Completed in 2017Q4 Nov 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment