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

WololoGoal purpleSheepPredicate is named wrongly #1344

Closed
i509VCB opened this issue May 10, 2020 · 11 comments
Closed

WololoGoal purpleSheepPredicate is named wrongly #1344

i509VCB opened this issue May 10, 2020 · 11 comments

Comments

@i509VCB
Copy link
Contributor

@i509VCB i509VCB commented May 10, 2020

The purpleSheepPredicate actually checks if the sheep is blue.

@iounpaladin
Copy link

@iounpaladin iounpaladin commented May 10, 2020

What it's doing is checking if the original colour combined with the new colour makes purple.

@mackycheese21
Copy link

@mackycheese21 mackycheese21 commented May 10, 2020

@Max1234567890 that can't be the case. It is a predicate on a sheep not two colors.

@iounpaladin
Copy link

@iounpaladin iounpaladin commented May 10, 2020

The second colour is always red. Hence it checks sheep->colour + red == purple (ie sheep->colour == blue)

@mackycheese21
Copy link

@mackycheese21 mackycheese21 commented May 10, 2020

Although that is true it is a misleading way to put it.

      private final TargetPredicate purpleSheepPredicate = (new TargetPredicate()).setBaseMaxDistance(16.0D).includeInvulnerable().setPredicate((livingEntity) -> {
         return ((SheepEntity)livingEntity).getColor() == DyeColor.BLUE;
      });
@i509VCB
Copy link
Contributor Author

@i509VCB i509VCB commented May 10, 2020

The second colour is always red. Hence it checks sheep->colour + red == purple (ie sheep->colour == blue)

That is a complete misunderstanding of the game's logic.

That TargetPredicate is used to test if the evoker should wololo the sheep, i.e. the Sheep MUST be blue:

List<SheepEntity> list = EvokerEntity.this.world.getTargets(SheepEntity.class, this.purpleSheepPredicate, EvokerEntity.this, EvokerEntity.this.getBoundingBox().expand(16.0D, 4.0D, 16.0D));
if (list.isEmpty()) {
    return false;

Also a further look into the goal shows the sheep is never meant to turn purple, but red as shown in castSpell

@iounpaladin
Copy link

@iounpaladin iounpaladin commented May 10, 2020

I know it doesn't turn purple, but the predicate, already knowing the sheep will turn red, checks if the original colour of the sheep combined with the new colour of the sheep is purple. Basically, the evoker is using a { return purple - x; } construction, but the evoker's favourite colour is red, so he only wants to turn sheep red. so he has to make sure that red = purple - the original colour, i.e. the colour is blue.

http://www.clipartbest.com/cliparts/aiq/Mb5/aiqMb5AiM.jpeg

@i509VCB
Copy link
Contributor Author

@i509VCB i509VCB commented May 10, 2020

Hmm, is there anything documenting that specific backstory? wiki for example?

I feel in this case, the logic should dictate the name, but a nice bit of humourous javadoc about that could be fun

@iounpaladin
Copy link

@iounpaladin iounpaladin commented May 10, 2020

Not directly, but given that a) it is officially stated that Evokers built redstone and b) the fact that Vexes are red as well is what lead me to that conclusion. It's also important to keep in mind the symbolic nature of the purple: the Evoker is the only Illager (including Vexes and Witches) not to have any purple on it, which leads to the idea that the Illager is "computing" purple - sheep's colour to turn them red.

@Juuxel Juuxel added the bug label May 10, 2020
@mackycheese21
Copy link

@mackycheese21 mackycheese21 commented May 10, 2020

Max, this should be put in documentation not the actual name. The name should be short and concise and reflect what it does.

@liach
Copy link
Contributor

@liach liach commented May 10, 2020

Lol why the heck are you concerned about implementation details if the implementation details are confusing and not helpful. Just call it convertibleSheepPredicate, why are you going to brainfart about a color nonsense

Also blame mezz for this thing. Yarn has been much smarter since luckily, naming things in more efficient ways.
5c6c0c7#diff-b6376ebd10e80e5e322fb28f766d47c4R44

@liach liach added the refactor label May 10, 2020
@mackycheese21
Copy link

@mackycheese21 mackycheese21 commented May 11, 2020

The details aren't confusing and its very clear what is being done though. It checks if the sheep is blue.

Juuxel added a commit to AntiquityMC/pomf that referenced this issue May 19, 2020
Co-authored-by: liach <liach@users.noreply.github.com>

Closes FabricMC#1344.
modmuss50 pushed a commit that referenced this issue Jun 7, 2020
* Fix #1369

* CriteriaMerger -> CriterionMerger

* ItemRenderer.renderGuiItem + related names

Closes #1361.

* WololoGoal.purpleSheepPredicate -> convertibleSheepPredicate

Co-authored-by: liach <liach@users.noreply.github.com>

Closes #1344.

* Improve GUI item rendering names

* MinecraftClient.(block/item)ColorMap -> (block/item)Colors

* Change wording a little

* Model overrides
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants