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

Implemented the /testfor command #573

Merged
merged 5 commits into from Oct 21, 2017

Conversation

Projects
None yet
4 participants
@smartboyathome
Contributor

smartboyathome commented Oct 15, 2017

As part of this, NBT tag matching was implemented so that the command can specify some subset of an entity's tags can be specified.

@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Oct 15, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Oct 15, 2017

CLA assistant check
All committers have signed the CLA.

Entity[] entities;
if (name.startsWith("@")) {
CommandTarget target = new CommandTarget(sender, name);
entities = target.getMatched(CommandUtils.getLocation(sender));

This comment has been minimized.

@momothereal

momothereal Oct 15, 2017

Member

Assuming the /testfor command can only be executed by physical senders (no console), you should validate that before (see other commands that use target selectors)

@momothereal

momothereal Oct 15, 2017

Member

Assuming the /testfor command can only be executed by physical senders (no console), you should validate that before (see other commands that use target selectors)

This comment has been minimized.

@smartboyathome

smartboyathome Oct 15, 2017

Contributor

I did some testing with the vanilla Minecraft server, and /testfor works fine from the console. Is there anything different I have to do in that case?

@smartboyathome

smartboyathome Oct 15, 2017

Contributor

I did some testing with the vanilla Minecraft server, and /testfor works fine from the console. Is there anything different I have to do in that case?

This comment has been minimized.

@momothereal

momothereal Oct 15, 2017

Member

If it can be executed by the console, I assume you can't use target selectors (like @e) as the console? IIRC you can only test-for players using the console.

@momothereal

momothereal Oct 15, 2017

Member

If it can be executed by the console, I assume you can't use target selectors (like @e) as the console? IIRC you can only test-for players using the console.

This comment has been minimized.

@smartboyathome

smartboyathome Oct 15, 2017

Contributor

After more testing, @e does work in the console. Just spawned a bunch of zombies into my vanilla server, and ran /testfor @e[r=500,type=zombie], and it gave me [Server: Found Zombie]. Looks like the command uses (0, 0, 0) as the location for the command when run in the console.

@smartboyathome

smartboyathome Oct 15, 2017

Contributor

After more testing, @e does work in the console. Just spawned a bunch of zombies into my vanilla server, and ran /testfor @e[r=500,type=zombie], and it gave me [Server: Found Zombie]. Looks like the command uses (0, 0, 0) as the location for the command when run in the console.

This comment has been minimized.

@momothereal

momothereal Oct 15, 2017

Member

That's interesting, I assumed it would take the target string as a literal player name if it's a console.

Did you test this with Vanilla or Spigot? Since Vanilla can only have 1 world, it makes sense to have this arbitrary location. But since Spigot (and Glowstone) support multiple worlds, this may not be possible (unless it takes the default world too?)

@momothereal

momothereal Oct 15, 2017

Member

That's interesting, I assumed it would take the target string as a literal player name if it's a console.

Did you test this with Vanilla or Spigot? Since Vanilla can only have 1 world, it makes sense to have this arbitrary location. But since Spigot (and Glowstone) support multiple worlds, this may not be possible (unless it takes the default world too?)

This comment has been minimized.

@smartboyathome

smartboyathome Oct 15, 2017

Contributor

I was testing with the Vanilla server. Just spun up a Spigot server, spawned in some zombies, and tested the command, works there too. Again, its centered on (0, 0, 0) in the default overworld.

@smartboyathome

smartboyathome Oct 15, 2017

Contributor

I was testing with the Vanilla server. Just spun up a Spigot server, spawned in some zombies, and tested the command, works there too. Again, its centered on (0, 0, 0) in the default overworld.

This comment has been minimized.

@momothereal

momothereal Oct 15, 2017

Member

Okay. So that means we should return 0 0 0 here:

and the default world here:

@momothereal

momothereal Oct 15, 2017

Member

Okay. So that means we should return 0 0 0 here:

and the default world here:

This comment has been minimized.

@smartboyathome

smartboyathome Oct 15, 2017

Contributor

Will do. Want me to clean up the other commands that use these functions and check for null, or do you want that to be in a separate PR?

@smartboyathome

smartboyathome Oct 15, 2017

Contributor

Will do. Want me to clean up the other commands that use these functions and check for null, or do you want that to be in a separate PR?

This comment has been minimized.

@momothereal

momothereal Oct 15, 2017

Member

That's up to you, and what scope you want to set for your PR :)

@momothereal

momothereal Oct 15, 2017

Member

That's up to you, and what scope you want to set for your PR :)

Show outdated Hide outdated src/main/java/net/glowstone/command/minecraft/TestForCommand.java

smartboyathome added some commits Oct 15, 2017

Commands run from console now center on (0,0,0) in the default world.
Replicating Spigot and Vanilla functionality for the /testfor command.
@smartboyathome

This comment has been minimized.

Show comment
Hide comment
@smartboyathome

smartboyathome Oct 16, 2017

Contributor

Requested changes made, and cleanup done.

Contributor

smartboyathome commented Oct 16, 2017

Requested changes made, and cleanup done.

@momothereal

This comment has been minimized.

Show comment
Hide comment
@momothereal

momothereal Oct 16, 2017

Member

Code looks good, I'll test the functionality tonight hopefully.

Member

momothereal commented Oct 16, 2017

Code looks good, I'll test the functionality tonight hopefully.

Show outdated Hide outdated src/main/java/net/glowstone/command/minecraft/TestForCommand.java
Show outdated Hide outdated src/main/java/net/glowstone/command/minecraft/TestForCommand.java
* @param other The CompoundTag that should contain our values.
* @return
*/
public boolean matches(CompoundTag other) {

This comment has been minimized.

@momothereal

momothereal Oct 18, 2017

Member

After testing the following command in my world with only 1 saddled pig: /testfor @e[type=pig] {Saddle:1b}, it still matched with pigs that weren't saddled. Either there is a problem with the logic in this function or with the way it is called in the command.

It would be a good idea to have a simple unit test to check if two compounds match using this function.

@momothereal

momothereal Oct 18, 2017

Member

After testing the following command in my world with only 1 saddled pig: /testfor @e[type=pig] {Saddle:1b}, it still matched with pigs that weren't saddled. Either there is a problem with the logic in this function or with the way it is called in the command.

It would be a good idea to have a simple unit test to check if two compounds match using this function.

@smartboyathome

This comment has been minimized.

Show comment
Hide comment
@smartboyathome

smartboyathome Oct 21, 2017

Contributor

Made the requested changes and fixed the bugs.

Contributor

smartboyathome commented Oct 21, 2017

Made the requested changes and fixed the bugs.

@momothereal

This comment has been minimized.

Show comment
Hide comment
@momothereal

momothereal Oct 21, 2017

Member

Thanks, will test asap.

Member

momothereal commented Oct 21, 2017

Thanks, will test asap.

@gdude2002

This comment has been minimized.

Show comment
Hide comment
@gdude2002
Member

gdude2002 commented Oct 21, 2017

Your changes have broken two tests. https://circleci.com/gh/GlowstoneMC/Glowstone/1210

@smartboyathome

This comment has been minimized.

Show comment
Hide comment
@smartboyathome

smartboyathome Oct 21, 2017

Contributor

Yeah, I realized that after I had pushed. Fixed them in my specific test, although I don't think that Tag.equals() will work correctly for ByteArrayTag and IntArrayTag, since Array.equals() will check for instance equality, while Arrays.equals() checks for subelement equality.

Contributor

smartboyathome commented Oct 21, 2017

Yeah, I realized that after I had pushed. Fixed them in my specific test, although I don't think that Tag.equals() will work correctly for ByteArrayTag and IntArrayTag, since Array.equals() will check for instance equality, while Arrays.equals() checks for subelement equality.

@momothereal momothereal merged commit f1d5f6f into GlowstoneMC:master Oct 21, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@momothereal

momothereal Oct 21, 2017

Member

Thank you for your contribution! :)

By the way, you don't need to squash commits together to fix something. Having them separated makes it easier to review, and we squash everything when merging anyways.

Member

momothereal commented Oct 21, 2017

Thank you for your contribution! :)

By the way, you don't need to squash commits together to fix something. Having them separated makes it easier to review, and we squash everything when merging anyways.

@momothereal momothereal referenced this pull request Oct 21, 2017

Open

Implementing vanilla commands #499

40 of 53 tasks complete

PickNChew added a commit to PickNChew/Glowstone that referenced this pull request Oct 25, 2017

Implemented the /testfor command (GlowstoneMC#573)
* Implemented the /testfor command, as well as NBT tag matching.

* Commands run from console now center on (0,0,0) in the default world.

@smartboyathome smartboyathome deleted the smartboyathome:testfor-command branch Dec 7, 2017

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