mcmonkeyprojects / Sentinel

Combat NPCs for Spigot!
MIT License
165 stars 85 forks source link

Sentinel causing lag because sentinel = player #259

Closed Momonche closed 6 years ago

Momonche commented 6 years ago

Hello !

I have a very big problem with Sentinel, I have a RolePlay server with approximately 800 Sentinel but a Sentinel is recognized as a player so

Sentinel have a economy account, and I have a plugin who take money on death, so when a Sentinel die, my economy plugin do a task I have some plugins for stats, so the server do some tasks on EVERY Sentinel non stop I have a combat indicator plugin, so when a Sentinel hit an other Sentinel, an hologram is displaying Ect... Ect... Ect...

So my server considers that 800 players on my server but that wrong ! And my server does not support more than 100 players so it's not possible to play !

Is it possible to consider a Sentinel like something other than a player ?

mcmonkey4eva commented 6 years ago

There's nothing restricting Sentinel NPCs to be only player-type NPCs. You can use any NPC type you please. Villager, zombie, skeleton, whatever you like, using the standard Citizens command for that: /npc type [TYPE HERE] like /npc type zombie.

You also might consider speaking to the developers of those plugins that are misbehaving like that to request they make it not misfire for NPCs.

Momonche commented 6 years ago

So if a random Guard is a "zombie" for type, nothing change for him ? He can have a sword or anything else, nothing change like a "player" type ?

mcmonkey4eva commented 6 years ago

For the most part just about everything should work the same, just the NPC will now look like a zombie instead of a player, and player-type-specific bugs should be gone.

Blamo27 commented 6 years ago

@mcmonkey4eva All plugins think that NPCs are players so they flood the MySQL database

Ferocimo commented 6 years ago

@mcmonkey4eva Obviously, with a roleplay point of view, we can't change all guards and NPCs into monsters just to avoid the issue. For instance, Sentry managed to avoid this problem somehow. The whole point of a plugin such as Sentinel is having NPCs that can attack, because you can perfectly configure monsters like you say with minecraft vanilla and command blocks.

mcmonkey4eva commented 6 years ago

@Blamo27 Not "all plugins". If all your plugins do that, then you have a lot of weird plugins that need fixing. You might want to contact whatever party developed the plugins affected about that. @Ferocimo I'm not sure why you think Sentry would in any way avoid the problem listed in this issue... but, no it didn't. Sentry and Sentinel both defer to Citizens for the entirety of the functionality regarding player-type NPCs functioning, which is the where the relevant issue here is from. It's possibly Sentry would have fewer of the effects listed in the OP, as Sentry simply did not integrate into Bukkit correctly anywhere, so many relevant Bukkit events would simply not fire, whereas Sentinel does actually integrate basic Bukkit/Minecraft code properly.

I don't know if all 3 of you are staff of a single server, or what the organization here is, but there's 3 people speaking as this issue is all their own.

If you are all from the same server: You have functionally incompatible requirements. If not all from a single server: Please treat separate issues as separate, don't mix into unrelated issues.

Ferocimo commented 6 years ago

We're all from the same server.

I'm saying that because we never had such an issue on Sentry : it might be because there was a more incomplete integration with Bukkit, but overall the result was more efficient regarding compatibility, somehow. I'm not saying this as a judgement, i'm just saying that, although I understand what you're saying, I also think that you might be able to do something on your end to avoid these issues with other plugins. Yes, these other plugins can probably fix the issue on their end, each individually, but honestly we've been developping our server and reporting issues to you for over two years now and constantly messaging every other developper for compatibility issues caused with Sentinel is EXTREMELY time consuming, and has litterally cost us one year (some developpers never answer and we have to fix the issue by ourselves).

So, I get what you're saying : if they worked better on compatibility, this issue wouldn't exist. Certainly. But since they simply won't, and since, if I'm not mistaken, you aim at developping a plugin that can actually work well on most servers, I believe it is necessary for that goal to look into this on your end. The other option (waiting for others to fix it on their own) is simply not viable anymore, at least on that matter specifically.

Regarding this issue, the main thing is that we have a plugin (CustomDeath) that takes money to the player upon death. But that plugin also tries to take money from NPCs when they die, because they are considered players and not just entities : they even have money accounts, from what I can judge. And the event https://hub.spigotmc.org/javadocs/spigot/org/bukkit/event/entity/PlayerDeathEvent.htm is fired. There are tons of other issues such as this one with various plugins but we've managed to get it relatively stable until now, there's just this one we can't fix.

mcmonkey4eva commented 6 years ago

I'm assuming that specific point is essentially: In Sentry, when an NPC got hit it removed points from a tracker held by Sentry, and when an NPC died the entity instantaneously disappeared from the world. In Sentinel, when an NPC gets hit it goes through all the same calculations as any entity is expected to within Bukkit and Minecraft. When an NPC dies, it goes through all the same calculations as it should.

The "fix" for the issues you have essentially would amount to adding more workaround config options that intentionally block Bukkit functionality because you have other plugins that interact with Bukkit in questionable ways.

I can do that "fix", but it is basically the least ideal fix for anything ever (whereas the ideal would be for the misbehaving plugins to be fixed).

Ferocimo commented 6 years ago

But what is the point for Sentinel to fire a PlayerDeathEvent ?

mcmonkey4eva commented 6 years ago

It doesn't fire that.

Bukkit does. Sentinel just doesn't forcibly block the event from occurring.

Ferocimo commented 6 years ago

So Sentry intentionnally blocked that event from being fired ? Were the NPCs on Sentry the same type than on Sentinel ? Player type ? Or were they regarded as undefined entities ?

mcmonkey4eva commented 6 years ago

They are both the same basic player entity type as that is handled entirely by Citizens, not Sentinel or Sentry.

I wouldn't say Sentry intentionally blocked that event, so much as it ended up blocking that event and others by chance of it breaking ten other things which broke those events as well, due to the dev involved not being particularly experienced.

Ferocimo commented 6 years ago

Okay, but for that matter, the question remains : what is the point of NOT blocking an event like that ?

mcmonkey4eva commented 6 years ago

What is the point of not preventing horses from spawning in worlds when Sentinel is installed? What is the point of not blocking chat messages containing the word "hacked" when Sentinel is installed?

While blocking things can be done, and may be exactly what some servers need/want, it's very often not needed or even actively disruptive to other server setups. When in doubt the default is to leave existing functionality the way it is until needing to do otherwise.

Ferocimo commented 6 years ago

I see, but in that case wouldn't it be good to offer an extensive possibility of customizing the plugin in config ? For instance, allowing the user to chose whether or not to disable this. That would make everyone happy in a way that everyone could choose what they want depending on their server setup.

mcmonkey4eva commented 6 years ago

I mean I did mention that I could exactly that about 8 posts up.

Ferocimo commented 6 years ago

Yes I saw, you also said that it would be the least ideal thing to do, but from what I can see it's the only thing that can be done for this issue, because waiting for others to fix it (although it would be more logical from what you say) simply is not an option at that point, it won't have anything solved.

So yes I would greatly appreciate if you could have a look into this "last resort" possibility.

BloodEko commented 6 years ago

This reminds me to a conversation I had.

Other games do technically differ between player-character objects (PC) and non-player-character objects (NPC). Minecraft doesn't. In Minecraft both are internally just the same: Player entities. That's why problems like this occur.

So to identify real npcs, plugins need to query citizens2, and check whether it registered the specific Player entity as npc. Thats our situation, yeah? I wish a overall solution for this would exist.

Blamo27 commented 6 years ago

@BloodEko Yes, we know it's because some events like PlayerDeathEvent is triggered when an NPC die for example.

I think that it's not possible to tell all developers to query Citizens ...

Maybe we can just suggest to Mojang to implement a customized non-player-character CNPC Entity (because NPC entities already exist).

mcmonkey4eva commented 6 years ago

You don't need to query Citizens to check if an entity is an NPC, there's a generic Bukkit method available for it. entity.hasMetadata("NPC") returns true or false to indicate whether an entity is an NPC.

mcmonkey4eva commented 6 years ago

Sentinel build DEV-181+ from http://ci.citizensnpcs.co/job/Sentinel/ contains an experimental fix for this. In the random section of the (new) config, set workaround bukkit events to true.

Blamo27 commented 6 years ago

@mcmonkey4eva Yes but, all plugins will not check if an entity is an NPC.

mcmonkey4eva commented 6 years ago

All plugins don't need to.

Only those that do things like interacting with a player database setup (economy plugins, some permissions systems, some types of restriction systems, etc). Most other plugins are fine just operating normally.

Blamo27 commented 6 years ago

@mcmonkey4eva I see that you only blocked death event, there's also the equip event for exemple. If an NPC equip an armor, this will fire the event.

Maybe you can put an array configuration, allowing Sentinel users to put the event that they want to block ?

mcmonkey4eva commented 6 years ago

That event is not even related to Sentinel. Equipping an NPC is handled within Citizens.

There is not some generic event blocking system, I used workaround logic to cause the death event to not fire when it can be caught in advance (by deleting the NPC's entity when it receives enough damage that it is about to die).

Blamo27 commented 6 years ago

@mcmonkey4eva Okey no problem, I am testing the fix

mcmonkey4eva commented 6 years ago

actually I realized belatedly there is literally not an equip event. That doesn't exist at all in Bukkit.

So as a further negation of your request: I cannot block events that don't even exist.

Blamo27 commented 6 years ago

Yes sorry, there is no armor equip event, but some event are able to retrieve this info

mcmonkey4eva commented 6 years ago

Can be sorta-achieved by listening to events like right click air, click in inventory, etc. as relevant... none of which would mistake an NPC for a real player.

If you encounter a specific actual thing happening that should be fixed, please let me know... I can't fix errors that you imagine, only ones that happen.

Blamo27 commented 6 years ago

It's not a bug that I imagine, it's a real problem. But it's not the Sentinel's fault.

Ferocimo commented 6 years ago

Tried with the fix, unfortunately the issue is still there, it still tries to take money to a Sentinel that dies.

mcmonkey4eva commented 6 years ago

Can you please update to Sentinel build DEV-187+ and see if it still occurs, and if so: Enable debug via /sentinel debug and get a proper log of the Sentinel getting killed to post here. (When debug is on it will output a mess of minor messages to the console).

Ferocimo commented 6 years ago

Sure, there it is : https://pastebin.com/7uPd2W2z

mcmonkey4eva commented 6 years ago

Woops, that's what I get for trusting enums to be used for their stated reason and not other reasons.

Should be fixed in build DEV-188+

Ferocimo commented 6 years ago

It is fixed, thank you very much !