NPC Draft #26
NPC Draft #26
Conversation
There was a problem hiding this comment.
Thanks again!
A couple things additionally to the comments:
- It would be good to directly provide a way to handle interacting with NPCs. Right/left-clicking on them seems like the most common interaction a player would want to do. Something like a
onInteract/setInteraction(Consumer<Interaction>)method or similar could represent such a function. - There currently is no difference between a spawned and non-spawned NPC which at least Citizens supports. I think that would be good to have too with
spawn/despawnandisSpawnedmethods. (maybe again with a Feature enum?) - A method to make the NPC move to a location instead of teleporting it might also be nice. Probably needs a feature flag again.
| package de.minebench.tresor.services.npc; | ||
|
|
||
| import de.minebench.tresor.services.npc.skin.NPCSkinData; | ||
|
|
||
| public class NPCMetadata { | ||
|
|
||
| private String displayName; | ||
| private NPCSkinData skinData; | ||
|
|
||
| public NPCMetadata(String displayName, NPCSkinData skinData) { | ||
| this.displayName = displayName; | ||
| this.skinData = skinData; | ||
| } | ||
|
|
||
| public String getDisplayName() { | ||
| return displayName; | ||
| } | ||
|
|
||
| public NPCSkinData getSkinData() { | ||
| return skinData; | ||
| } | ||
|
|
||
| public void setDisplayName(String displayName) { | ||
| this.displayName = displayName; | ||
| } | ||
|
|
||
| public void setSkinData(NPCSkinData skinData) { | ||
| this.skinData = skinData; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
I'm unsure if it needs a separate class for this, especially if normally one associates dynamic, user-provided data with "metadata" in the Bukkit API.
Imo. this could easily be just on the base NPC interface and the values directly returned by the wrapper. (Which would even avoid constructing a new metadata object every time someone wants to get the NPC name. Also some people could be confused as to how to get the name if it's not directly on the NPC itself)
So unless there is some reason behind making that a different class (like some implementations doing something with metadata I'm not aware of) I would prefer these methods to be directly inside NPC.
There was a problem hiding this comment.
The main point of this class is separation of concerns. It also allows you to copy over metadata and apply it to multiple NPCs.
There was a problem hiding this comment.
Hm, the copying is a good idea but I feel like exposing a separate copy method for all of the NPCs data (even one not exposed by the service but only internal to the implementing plugin) would be better. (And only separating it for two values feels a bit over the top tbh. unless you had the idea to add more than just those two?)
There was a problem hiding this comment.
The idea is indeed to add more fields, such as glowing status etc.
There was a problem hiding this comment.
Hm, I guess then it would make sense to keep separate. Not sure if two ways of copying data is good though (one just the Tresor metadata, the other with all internal data if the implementation supports that... not exposing it like this would discourage copying of the data via the metadata which might be good as someone could think it also copies internal metadata)
There was a problem hiding this comment.
Hm, I guess then it would make sense to keep separate. Not sure if two ways of copying data is good though (one just the Tresor metadata, the other with all internal data if the implementation supports that... not exposing it like this would discourage copying of the data via the metadata which might be good as someone could think it also copies internal metadata)
Any idea for a suggested fix? I could make this a mutable object with dirty state checking but it'd end up being pretty hacky. Renaming the "setData" method to "updateData" will still allow for copying, Embedding the data in the npc class might work but I'm afraid it'll become a god class and be responsible for too much.
There was a problem hiding this comment.
I think it would be better to just move everything in to the NPC class. I don't think it risks being a god class because most stuff wouldn't actually be behaviour logic, just "field" getters and setters. (It would also make it easier for plugins to implement the service provider. Adding too many interfaces which one would need to implement to support Tresor might discourage some from supporting it natively especially when the traditional Economy etc. providers only required a single interface to be implemented)
Then a separate NPC#copy(NPC) method could be used to have the implementation handle the copying of all of the data (or what the implementation thinks all the data relevant to an NPC is).
api/src/main/java/de/minebench/tresor/services/npc/skin/NPCSkinData.java
Outdated
Show resolved
Hide resolved
providers/citizens/src/main/java/de/minebench/tresor/providers/citizens/CitizensNPCs.java
Outdated
Show resolved
Hide resolved
|
@Phoenix616 I've gone ahead and pushed some changes. Let me know if this resolves some of your feedback. |
|
Looks good overall, I like the approach with the interaction object also including the item as that's probably what one wants most often. (Although I guess that opens one up to main and offhand issues but I feel like if that is important someone could just get it from the player directly) One feature that I think might also be interesting to have is per-player NPCs (seeing as Citizens supports that naively it seems like something one would want to) as well as maybe the ability to get the raw entities connected to the NPC? (Not sure if that is even something needed or good to add here though seeing as some plugins might be doing client-side entities...) |
Citizens doesn't provide a way to get the hand used in the interaction. I feel like it should be an implementation detail.
Not every NPC plugin allows for client-sided NPCs. We should probably put that behind a Feature enum |
|
Yeah, hand makes sense to keep internally and only expose a generic left or right "interaction" as most probably don't care (or even know :S) about the both hand interaction stuff. As for the client-side NPCs: I really don't know how common that is tbh., I don't know an example of that myself but a method to get the NPC entity(ies) is probably a must. (Even if it just returns an empty list for plugins that use client-sided ones) |
I'm unsure if a list fits this scenario. We'd be better off coming up with a Model system in the future. Maybe a nullable entity getter? |
I made a pretty rough NPC system. This is some serious 3AM code, so I'm expecting some changes.
Closes #23
IssueHunt Summary
Referenced issues
This pull request has been submitted to: