search

juliarn / npc-lib

Asynchronous, high-performance Minecraft NPC library for 1.8-1.21 servers.
MIT License
320 stars 50 forks source link

Add exclusive mode to an NPC #45

Closed Cryptite closed 1 year ago

Cryptite commented 3 years ago

Introduces exclusive-mode (opposite functionality, basically), wherein an NPC can be created and is hidden from Players by default. Players will then need to be explicitly included to be seen, vs managing an "all-but-one" exclusion list.

This works in my local testing, though the verbage and naming of things is a bit hard to understand. I tried to keep with the API concepts so as not to break anything.

It seemed wise to make an NPC immutably exclusive since, once you choose to do exclusive, you have to use the opposite calls for checking visibility, but if you wish to make exclusive a mutable concept on an NPC, go for it.

derklaro commented 3 years ago

I just looked over the PR an it looks a little bit weird to me. First of all there are now 2 sets which are used instead of one. That makes no sense as your code always checks and writes only against one of the sets. The other is redundant and can be removed - this might require some other changes. I think it might be a good thing if you can specify a predicate which decides wether a player should be included (or excluded based on the mode) instead of letting the user always call one of the methods instead (but it might be stuff we can introduce in another PR). On the other hand this change makes it more difficult for developers to work with npcs, especially if they dont know if a npc is exclusive or not - there should be one shared method which either in- or excludes the player.

Overall a good impression tho.

Cryptite commented 3 years ago

I believe I may've added the second list mostly to not break API as one list would've involved creating a lot of strangely "inverse" method calls. All for a different idea on how this should work, but mostly just wanted to take a stab at it.

I agree for simplicity it should be one list/calls, but wasn't quite yet willing to rewrite most of the logic to support it. Feel free to close if you'd like to tackle this with a different methodology.

derklaro commented 1 year ago

@Cryptite I'm closing this for v2 now. In v3 I added the possibility to define a tracking rule which allows you to define (per npc) which players should get tracked. The api still has the possibility to force track a player, if you only want to track players added by the api. I hope that resolves this pull request and the general issue you had in v2.