notnotnotswipez / TerminatorNPC

6 stars 7 forks source link

Rewrite #1

Closed Daxanius closed 2 years ago

Daxanius commented 2 years ago

I have rewritten many of TerminatorNPCs systems in a way, which according to me, are better implemented and more polished. While it's still not perfect and can still use some changes here and there, I feel like it's a lot better from what it was before.

What I've done

Overhauled the command system

I've overhauled the command system to make use of Citizens2's command system, which is included in their API. That way you don't have to write your own command system, while also maintaining an easy to use and lightweight command system.

Add ignore

I have added the option for terminators to ignore specific players, that way you can do lots more interesting stuff with the plugin. The implementation is a bit questionable though, you might want to look at that, it was rather painful to get this to work as intended.

Add a create and destroy summoner command

You can now create summoners, which will automatically summon a terminator every x ticks at the location you ran the command at. You can create summoners and destroy them with ease, they might make for some interesting and fun game-play as well.

Bug fixes and improvements

While these changes were meant for a personal project, I believe TerminatorNPC could greatly benefit from them, which is why I am making this pull request. It is up to the creator of TerminatorNPC to decide if they want this or not.

notnotnotswipez commented 2 years ago

Oh cool, I’ve just looked this over. Not bad, but a few things I noticed.

1) Fixing the distance is neat, however it’s a bit pointless, as Citizens makes you set a hard cap for the targeting distance anyway (In this case, 100 blocks).

2) A little concerned with the multidimensional tracking change, I don’t remember how I had it set up, but have you tested whether or not the terminator will follow you between dimensions? It seems like it no longer tracks between worlds.

3) Although its a nice PR, I don’t think its substantial enough for you to be listed as an author on the yml. Unless its just something you forgot LOL, in which case no big deal. (If this gets merged ill mark you down as a contributor on the spigot page)

notnotnotswipez commented 2 years ago

The ignoring and new command system I really like, good job. Will have a closer look in my IDE later and a test soon :) Thank you for the PR

Daxanius commented 2 years ago

About the distance, I didn't know citizens has a hard caps for it. Well, at least it's not capped by itself now, also while using this myself in some tests the other day it did seem to work for the "infinite" range. I could spawn in a terminator, fly a couple thousand blocks away, go in survival, and the terminator would show up, while before the change it had some issues with that.

You might be right on the multidimensional tracking, unfortunately for me alone, this is quite hard to test. You might be better suited to take a look at this tbh, I had my share of issues working with the tracking / AI system.

Yeah, I forgot removing myself as author from the yml, I did it back then on a whim, then forgot about it.

I appreciate you taking the time out of your day to check this out :-), very cool

notnotnotswipez commented 2 years ago

The reason the terminator showed up is because it teleports to you, when you get far it never walks towards you. It teleports to wherever you arent looking after a random amount of time

Daxanius commented 2 years ago

That is if the terminator has a target yes, but the previous targeting implementation only targets players within a 1000 block radius. If you were not targeted before, and you are outside of that radius, it would not be able to teleport to you. This becomes especially clear if you make use of the ignore feature the way it is implemented now.

notnotnotswipez commented 2 years ago

Ohhh thats a good point, but it shouldnt matter if the target is set already and the NPC is already tracking you. It only overrides the current target if it found a better alternative

Daxanius commented 2 years ago

Yep, but that's the behavior we want, is it not?

notnotnotswipez commented 2 years ago

Yeah, thats fair

Daxanius commented 2 years ago

I removed myself as an author from the yml in the latest commit, as it was unnecessary.

notnotnotswipez commented 2 years ago

Looks good, tested, dimension hopping works fine, they just lose you when you die in the dimension and respawn in another one. (Which makes more sense), Merging 👍