-
Notifications
You must be signed in to change notification settings - Fork 6.2k
Scripts/Naxxramas: Thaddius: Complete rewrite #15523
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There are many wrongs in this PR :( Code style is a train wreck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional missing break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think its better to call this on reset as its also called on spawn.
leave constructor to initialize variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scriptname wasn't already assigned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was, actually. Should I remove the statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The update commands don't do any harm, they are just redundant if the names are there already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, figured I'd put them in for everything that has an AI from this script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls remove unneeded sql queries
how many stuff is confirmed with sniffs? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain this part, the grid is very likely to be active while the players are inside the room
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just trying to prevent weirdness if the last person to drop from combat is also the last person that hasn't released and leaves grid just as they exit combat (hearthstone etc.)
BossAI does it using the default _EnterCombat, too, which is where I got the idea.
dcc5110
to
2bd2113
Compare
I would consider this PR ready for testing, maybe with static/dynamic analysis tools too |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this value allow anyone to locally merge this PR and try it out ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It merged fine on a clean TDB for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I told him to use such huge guid to avoid travis blaming because SET @Cguid = XXXX; and be sure it don't uses existing guids :P ofc when merged it must be changed to unused guids on range 1-213000.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kk, so anyone can test this PR ingame and provide any feedback
Any test results are much appreciated. I tried pretty much every constellation of weird encounter resets that I could think of and the encounter never broke, but I'm sure someone else can come up with something that I didn't think of. Also, please report stuff, even minor stuff, that doesn't match retail if you have retail experience. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will bug out if the damage is exactly equal to his health (rare cases, but happens).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it just make him "die" if damage is equal to his health (which is what's intended)? I may not be seeing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, nevermind that, I got the logic now
42c765d
to
2d0931f
Compare
2d0931f
to
ed0551f
Compare
ed0551f
to
f8508b7
Compare
Fixed SQL to conform to standards, and moved texts to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove c style
c7b81e8
to
fecb007
Compare
fecb007
to
c8d2ae5
Compare
Moved GUIDs again to be unused 2 GUID slots. |
c8d2ae5
to
80f339f
Compare
Rebased. |
9646fd3
to
487140d
Compare
487140d
to
29feae5
Compare
Scripts/Naxxramas: Thaddius: Complete rewrite
Full rewrite of the Thaddius encounter. Stuff fixed, among others:
PS: My first full rewrite of a boss AI. Some stuff may be done incorrectly. Please tell me how to do it better. Thanks.