cheahjs / TerrariaAPI-Server

Fork is now over at https://github.com/NyxStudios/TerrariaAPI-Server
https://tshock.co
32 stars 24 forks source link

NPC Spawn Event not returning position. #47

Closed GitFlip closed 9 years ago

GitFlip commented 9 years ago

I am making a TShock plugin and I am using the NPCSpawn hook. I can correctly get the NPC, but it's position (x,y) is always (0,0). Is this a bug or intended?

https://github.com/Deathmax/TerrariaAPI-Server/blob/d446a8288a5d7984f4298a507a64249865259472/TerrariaApi.Server/HookManager.cs#L632

public override void Initialize() { ..... ServerApi.Hooks.NpcSpawn.Register(this, OnNpcSpawn, 5); .... } .... public void OnNpcSpawn(NpcSpawnEventArgs args) { NPC npc= args.Npc; Log.ConsoleInfo("NPC " + npc.type + " Spawned at " + npc.position.X + ", " + npc.position.Y); }

NPC 61 Spawned at 0, 0 NPC 69 Spawned at 0, 0

Ijwu commented 9 years ago

You're being passed a reference to the NPC object itself. If it reports a position of (0,0) it's most likely not due to TS-API.

Are you sure the NPCs are not being affected by another plugin? For example, I believe DieMob regions cause NPCs to appear at (0,0) then die or something like that.

GitFlip commented 9 years ago

Right I agree with what you are saying which is why I got very confused when the console was saying they are spawning at (0,0). I should also say they are definitely not spawning at (0,0) in game. I confirmed that the mob type was correctly spawning around me in game and not at (0,0).

The only plugin I have installed is the one I am working on which is not manipulating the NPC spawning ( yet because I cannot get their (x,y) ). The DieMob regions doesn't do anything to the NPCs that spawn. It checks if they are within a certain region on the GameUpdate Hook and kills them if they are in a region: https://github.com/InanZen/DieMob/blob/master/DieMob.cs#L166

Is there any chance you can make a quick tshock plugin where you simply use the NPCSpawn Hook for yourself and print out the same output as me to verify that it is on my end and not something with TS-API?

Thank you

Olink commented 9 years ago

Why not just look at the source code and determine the issue??? Its open source for a reason.

hakusaro commented 9 years ago

+1 to Zack

On Tue, Oct 28, 2014, 11:55 AM Zack notifications@github.com wrote:

Why not just look at the source code and determine the issue??? Its open source for a reason.

— Reply to this email directly or view it on GitHub https://github.com/Deathmax/TerrariaAPI-Server/issues/47#issuecomment-60800925 .

GitFlip commented 9 years ago

Not sure it's a +1 really... I am trying to determine if this is an issue with TS-API or my plugin. I have looked at the source code a little bit, and actually do not see a problem because it is just like Ijwu said.

Can't we just determine first if it's me using the API incorrectly or if it's the API before spending any more time looking at code? You know that's usually how you solve bugs - you reproduce them. Anyone of you willing to confirm that it's not a bug in TS-API and I'm just wrongly using the hook? After we get through that I am 100% willing to help fix the bug since I kind of need this feature to work in my plugin, but like I said I would like confirmation that a bug even exists.

Ijwu commented 9 years ago

I'll test it out as soon as I have a chance. For now I'm off to class.

hakusaro commented 9 years ago

@GitFlip well last I heard, Zack doesn't really have all the time in the world to check things like this right now, and I'm also at uni doing lots of stuff, so really, in this specific instance, what with the project being open source, self support is probably the best bet.

@Ijwu thanks for helping test this.

GitFlip commented 9 years ago

@nicatronTg I completely understand. I just graduated from Uni a couple years ago and now I work full time as a software engineer. In my very little free time I am working on making a Terraria MMORPG Style mod by releasing my own custom client and using tShock for the server modding. I've already decided I will spend some time this week looking into this more which obviously includes looking at the TS-API source code since, well, I need this functionality.

@Ijwu I appreciate getting the extra testing. I know it can be a pain to set things up, but it is really simple to reproduce if there is a bug, again I great appreciate the confirmation from someone besides me.

With all of this said I've been very happy with tShock and TS-API, so thank you.

QuiCM commented 9 years ago

@GitFlip @Ijwu Some brief searching in Npc.cs shows that NPC position is assigned after the api hook is invoked. http://puu.sh/cupcl/baeb1f3c66.png

Hope that helps in some way

GitFlip commented 9 years ago

@WhiteXZ Thank you, you are exactly right. This would result in x and y to always be zero. I've never contributed to an open source project on github, but I am more than willing to make the code change to move the hook after we set the position. Does anyone else think that we should move it somewhere else, like right before we return?

Ijwu commented 9 years ago

Sounds good. Thanks @WhiteXZ.

@GitFlip If you know how to use Git, fork the repo in GitHub then clone the subsequent fork to your machine. Make your changes and update your fork using Git. Then you use GitHub to make a pull request.

Simply move the hook invocation to after the position is set, but not after active is set. This should let the hooks retain their ability to properly deny NPC spawning should they need to.

P.S. Come by the TShock forum and post about your mod in the off-topic area. I'm sure some of us would love a good discussion.

GitFlip commented 9 years ago

@Ijwu Thank you for the explanation, I've mostly only used github for my own projects, and never actually forked, or did a pull request to another project. I'll try to make the change tonight or tomorrow.

I'll also get around to posting in the off-topic area like you suggested, because I'd love to discuss it with you all. In the meantime feel free to view my post on the new official Terraria Forums: http://forums.terraria.org/index.php?threads/super-terraria-world-mmorpg-style-mod-server-and-client-mod.936/

Olink commented 9 years ago

Fwiw my reply to look yourself was due to the following things you are likely unaware of. 1: This project is in maintenance mode and has been for a long time. We have no obligation to answer any questions and are very welcome to suggest you dig into the source code. 2: I know how lazy and useless Ijwu and White are, had I not said something relatively aggressive they would never seriously answer your question. My answer not only was correct, but it got you the answer you ultimately wanted. 3: Assuming you want to do anything remotely fun in Terraria you will need to get used to looking at the source code, for better or worse. This project is not for the faint of heart, and if my reply stopped someone unknowledgeable from constantly asking for help I am willing to suffer someone knowledgeable being put off. Unfortunately this is the type of community terraria brings and its a pita dealing with 12 year olds all day, something you don't get by being an adult and working in the industry.

Hopefully you understand I wasn't being cross with you, and you can understand where I am coming from. You are more likely to get help on the forums for this type of question anyways, hahaha.

Honestly without digging into the code i wouldn't know why the hook is where it is, so my suggestion is to just pass the x and y to the event and not change the placement of the hook imo.

E: nicatronTg; typos

GitFlip commented 9 years ago

@Olink Thank you for the explanation, I do understand where you are coming from - at least to some degree.

@WhiteXZ I hadn't noticed that you made the fix that Olink had suggested. I went ahead and made what I think is actually the correct fix - moving the setting of the positions to right before we invoke the hook. You can view my pull request here: https://github.com/Deathmax/TerrariaAPI-Server/pull/49

Olink commented 9 years ago

3ed336cb3fd50dbf796d9478e0a8d3c123ec92d9 solves this issue