search

cmangos / issues

This repository is used as a centralized point for all issues regarding CMaNGOS.
179 stars 47 forks source link

[CLASSIC] escort quest "Bodyguard for Hire" - not attacked #1693

Closed RichardGe2 closed 5 years ago

RichardGe2 commented 6 years ago

During quest 5821 : Bodyguard for Hire , the spawned creatures rarely attack the caravan.
Both caravan NPCs vs attackers NPCs seem to ignore themselves. ( However, my character is attacked - but if I'm far enough, the caravan can travel without any attack) I assume this is a simple issue related to faction assignment... but I don't know how to fix that.

My source and database are several months old... but I don't see recent fixes related to this issue, so I assume the bug still exists.

To reproduce: start the server and go to Desolace. you have to wait ~15 minutes before Cork Gizelton moves the caravan and asks for escort help. As soon as he asks help, you can take and start the quest

cala commented 6 years ago

The ambushing NPCs were attacking the caravan when I released the script (I tested it) and the factions were not changed. So I assume some intermediate change broke the relations between the NPCs.

killerwife commented 6 years ago

90% of these issues are due to UNIT_FLAG_IMMUNE_TO_NPC not being removed correctly in script.

cala commented 6 years ago

I think you meant:

90% of these issues are due to UNIT_FLAG_IMMUNE_TO_NPC ~not being~ that now needs to be manually removed in script because the behaviour was changed in core.

because this script, like the Stitches one and many others were working even if the behaviour core-side was incorrect (bad handling of the flag though we did not know it). The flag was changed in core (I agree the change is correct) but not only the documentation was not fully updated leading to misunderstanding, the change broke many scripts and quests. This change happened months if not a year ago and we are still seeing fixes/issues related to it almost every week.

We are in a concentric ecosystem: the core is a tool for DB devs/scripters to do their job and they cannot always see all the side effects of a core change, especially DB devs as they have even fewer ways of scripting compared to SD2 devs. For example, you will understand that DB devs like Grz3s or myself cannot review the thousands of commits we made over the years every time a big change occurs in the core.

In the present case, if I had not seen by chance the Stitches script fail or if this report was not made, I would have never known that the scripts were no longer working correctly because they have been previously reworked and I would have no reason to look at them again. How many other scripts/quests are in the same state?

I'm probably not the first one to tell this but you have a tendency to think that because your changes are valid (at least in your point of view) this is for other people to handle the consequences of these changes.

We cannot ask of a full backward compatibility of course, but it would have been better to have a way to spot things affected by this change or at the very least new errors reported in logs so we can easily find what's wrong/needs to be updated. I remember that when Schmoozerd did changes that affects DBs, he provided tools and often scripts to help DB devs to handle the changes. It was very helpful and helped to roll out changes more efficiently.

killerwife commented 6 years ago

Well, if you looked in tbc-db, you would know that we fixed more than 70 issues like this, possibly even more, and even in core I ported many to cmangos that fix this exact issue. I understand that it might have been badly communicated by you, but me, WB spammed it a lot on discord, and Grz3s knew about it.

There was no way to do backwards compatibility on this, it simply broke quests. The upside is that you can mimic sniffs 1:1 now and it works. The way it was spotted on our end was to launch it on voa. There is no other possible sane way of finding these issues other than rigorously testing all old scripts. Proof we are fixing these issues even for vanilla in tbc-db: https://github.com/cmangos/tbc-db/commit/c7a159feba7d2652e13ccc2f298eea12a02178ad

cala commented 6 years ago

I don't deny you and your team have fixed some of these issues, like the ones you've ported from Light's Hope server. I've done my share too. We are not doing an e-peen contest here or counting who made the more commits.

I understand that it might have been badly communicated by you, but me, WB spammed it a lot on discord, and Grz3s knew about it.

We already had this discussion. You still don't understand or don't want to. First, you keep thinking of discord as a sustainable tool for communication. It is great for discussion but we can't scroll through hundreds or thousands of chat lines when looking for important information. I can't spend my whole time on discord, even WB or cyberium do not do it. For documentation, we have a wiki for that purpose, that is accessible to every user at any time, which is not the case for all discord chans. And the fact is that the wiki is not up-to-date. Secondly, I am well aware of what WB said and did. I even challenged him on the implementation of his Relations API for some Classic-specific cases and he made the changes after we discussed them.

The upside is that you can mimic sniffs 1:1 now and it works.

Again, I'm fine with the change by itself and I understand what it does. I'm perfectly fine with the API and what it allows. I'm not fine with the way it was rolled out. Even though the change was made long ago, that dozen of fixes were made as your are yourself proving, we are still discovering things broken by this change now. This is fact. This is my whole point.

It would have been better to find a way at that time to automatically spot scripts hit or potentially hit by this change and prompt warnings/errors even with false positives so we can get rid of this once and for all instead of the current situation where some are fixed and some others aren't and we don't know which ones.

killerwife commented 6 years ago

Well I am not able to make any such system that wouldn't have 10x more false positives. The problem is also how we (understand my team) are tackling this issue. We tackle it by looking it up in sniffs, and this process is mostly a 2-3 per day. Again, this is not meant to say how many we fixed, just that its time consuming, and why it is what it is.

When it comes to wiki, tell me whats not updated and I will sort it out with my team. (just disregard EAI please for now)

I know you can't pay attention to discord, neither does cyberium, or grz3s, but at this point even I am not paying attention to it enough. I have like 5 people messaging me who filter through stuff and how it gets to me. If there is a format which is feasible to update to pass these changes along, then someone needs to show it to me because I do not know how. I tend to ping everyone on critical commits and so far noone usually responds for weeks. Same can be true for @Saltgurka cos he complained to me that noone replies to him either. So clearly all of us are doing smth wrong.

Saltgurka commented 6 years ago

I just want to pitch in that the quest was fixed in this commit 6 days ago: https://github.com/cmangos/tbc-db/commit/49dc8f97fce183728a4475eabf30d98448a6ebde

The problem was not the UnitFlag itself but that the TEMPFACTION flags were wrong (UnitFlags were restored upon leaving combat)

As for the whole discussion in this thread, I won't get into it too much. In my opinion what we must strive for is to have a core that allows us to follow sniffs 100%, and the update to TEMPFACTION_TOGGLE_IMMUNE_TO_NPC was one huge and important step in that direction. Every big change will always cause regressions and to be fair this one only affects scripts that were not scripted correctly to begin with (or in this case simply contains a minor mistake/typo).

As for communications, we all know we got an issue there and I'm still not sure what our official main communication channel is. Discord? IRC? Github? My vote would be on Discord simply because I use it daily and if we set it up properly we could have a "News" channel where all major changes like this could be announced.

cyberium commented 6 years ago

Core devs/Scripters and DB devs are different things with different objectives. But we have one main goal. Improve our project and make it better. So we all have to be patient. As a core dev we have to listen content devs and vice versa. Its not always easy.

About the communication @cala is correct, we lack clearly on a more reliable way to discuss about some specific dev project. Iam open for some suggestion, do we need to reopen a forum? A disqus? Something that can let us the chance to talk about what we think its important. Wiki, discord and github are not for that. (well there is some new feature in github that i have to explore though)

RichardGe2 commented 6 years ago

@Saltgurka - good to know it's fixed. however your fix seems to be targeting TBC only ? I don't know how you guys manage deployment on all the other wow versions - but I assume this fix should be copied to Vanilla.

Grz3s commented 6 years ago

Everyone will agree, that is not nice to find out ... that script u worked on (few days on one script etc) ... after few years .. just stops working .. and you have to spend more time to find out what happened... Massive change for 0x100 and 0x200 ... is a good example of that... even xfurry complained that this change will dmg. lots of sd2 scripts...

From other side ... We all know that Core is not perfect and devs slowly step by step making it better. Sometimes this will brake stuff ... but we all know about it. As a good example: when Schmoo.. added new buddy system i started to making tons of new scripts using it ... and few months later he change whole logic (buddy - other buddy) - I had to redo 100+ maybe more... scripts one by one... (it wasnt nice - but i knew its right way). The only diff ..was that he told me whats commig, so i had a time to prepare all.

Sometimes would be nice .. when some big change commig (that core dev knows it will brake smth)...just let everyone know (we do have nice tool on discord for that) ..

RichardGe2 commented 6 years ago

I tested the fix pointed by @Saltgurka on my Classic server - I confirm it's solving the issue. not perfect, because sometime Cork Gizelton continues to walk even if the caravan is under attack - I assume he should come to help. but this is another issue - and more minor.

RichardGe2 commented 6 years ago

...and if Cork Gizelton reaches the finish point alone - we validate the quest - even if all the caravan has been killed - main fix for that would be to force Cork Gizelton to come help us during fights, instead of continue his way

cala commented 6 years ago

@RichardGe2 Yes, the fix will be ported. I have technical issues preventing me from pushing right now, but I intend to port recent changes from TBC and WotLK DBs to Classic DB. I will also handle a few PR currently pending. Regarding the quest failure, it is not properly handled at the moment, I have to look in the commit history why I did not add it, but I remember leaving a note there on that purpose for later.

@ everyone: thank you for the feedbacks and suggestions. We are going somewhere. Regarding the communication between us, before looking for what tools, we should clarify our needs:

I can discuss with the guy in charge of code quality and documentation at work to see if he has some advices, because we had similar concerns in the past. I also propose to write myself drafts in the wiki regarding the core systems, so we can build upon it and everyone here could correct/enhance them.