BuffStackTest (MQ2Main/MQ2Utilities.cpp)

[demonstar55]

23514 with 18075, 18066 (this has other issues as noted above), 25959, 25999.

Note: These are returning FALSE where they should return TRUE. And I did revert my changes and double checked them all.

http://pastebin.com/1xrCrpui

[Cr4zyb4rd]

My comments below are directed at everybody involved/interested in this but the quote encapsulates it all nicely:

Not really necessary as I have a database of all the spells... but thanks anyway.

Then share it and use it instead of this ridiculous code. What's obviously needed here is a table lookup. It's not a matter of formatting, this code is flat-out doing it wrong, attempting to encodify people's gut feeling that "there must be some order to it" to the stacking rules. Sure there is. We have table made by a human based on intuitions and some ideas about how things "should be" with some templating and systematic rules applied later.

Step back. Look at what we're doing and where we're doing it. Here are some other functions defined in Utilities: WriteChatf, StrReplaceSection, GetNextArg, CompareTimes seems fairly obvious to me that these were all intended as short utility functions made for use elsewhere in our code. Sure there's been some creep as the base expanded, but this is ridiculous... it looks like somebody cut-and-pasted half of MQ2Melee into otherwise tight C-like utility code.

Step back. Look at this and the other threads talking about these stacking tests. We can barely even discuss the test cases with each other because our own rules make no sense.

This is simple tabular data: each spell has other spells it doesn't stack with and we need to check that list for a given spell. We can abstract that all into numbers and check vs spell IDs, or we can do "slower" string compares (still literally millions of times faster than I can do myself when I ask "can I cast this on him?") We already have a powerful text parser at-hand, a full macro language in fact.

If you've taken this all in and still somehow thing that this is the right way to go about things, at least write maintainable code. Use defines and constants, not magic numbers. If you're testing multiple cases, define intermediate values with meaningful names and use them instead of a single multi-line if statements with a sea of ANDs and ORs. Train yourself to do this type of thing and I promise you'll spend more time writing creative code as the bugs and edge-cases naturally fall out without needing to field test.

[SwiftyMUSE]

Then share it and use it instead of this ridiculous code. What's obviously needed here is a table lookup.

Are you really trying to say we should include an actual database runtime in the base MQ2 code? That in itself would make this unusable and unmanageable for the average user.

Now, if you are saying to share the database to have an easy way to look up things OUTSIDE of the code, that is currently possible. Just download the information from Lucy and import into your favorite database. Those that would use a database should have the knowledge to do this themselves.

Let's stop talking in the abstract about how the code should be structured. As an open source project. I personally will try to maintain code, IN THE SAME FORM, as was originally written. This provides the ability to more easily understand the code as you don't have multiple coding styles competing with each other. Sure some of the code could be cleaned up, but I would say, more people would prefer the devs adding NEW functionality than doing code cleanup which doesn't benefit the vast majority of the users of MQ2.

[demonstar55]

MQ2 doesn't need a DB I'm assuming his databases is just EQEmu. We do have some nice scripts to import the spells file into the DB if you want a nice queryable form to look at the spell data in.

[Cr4zyb4rd]

It's hardly abstract when I have the code in front of me, here in the bug reports thread about it.

What I'm "really" trying to say is in the text I posted, which is where I usually tend to include such things. It's a reasonable practice, and one that I'm encouraging others to adapt in their coding habits.

Code: Select all

          if (!((bSpell->Attrib[i]==10 && (bSpell->Base[i]==-6 || bSpell->Base[i]==0)) ||
                (aSpell->Attrib[i]==10 && (aSpell->Base[i]==-6 || aSpell->Base[i]==0)) ||
                (bSpell->Attrib[i]==79 && bSpell->Base[i]>0 && bSpell->TargetType==6) ||
                (aSpell->Attrib[i]==79 && aSpell->Base[i]>0 && aSpell->TargetType==6) ||
                (bSpell->Attrib[i]==0  && bSpell->Base[i]<0) ||
                (aSpell->Attrib[i]==0  && aSpell->Base[i]<0) ||
                (bSpell->Attrib[i]==148 || bSpell->Attrib[i]==149) ||
                (aSpell->Attrib[i]==148 || aSpell->Attrib[i]==149)))
                return false;

What's at issue is what we are "really trying to say" here. Read this aloud, I dare you. Whether we choose to call it a database or reserve that word as a scarecrow to throw rocks at is our choice, but it's currently being handled ineptly.

[demonstar55]

More spells with issues: Avatar line and bard haste line. Specifically Champion (5427) and War March of Brekt (18063)

This isn't the same issue that I mentioned originally, but with the whole beneficial spell and song should stack even if they have conflicting slots issue that I also mentioned.

This is returning FALSE when it should return TRUE

[SwiftyMUSE]

More spells with issues: Avatar line and bard haste line. Specifically Champion (5427) and War March of Brekt (18063)

This isn't the same issue that I mentioned originally, but with the whole beneficial spell and song should stack even if they have conflicting slots issue that I also mentioned.

This is returning FALSE when it should return TRUE

This is the reason I resisted putting songs (shortbuff window) into stacks before. It is specifically failing because of slot 3 increase ATK.

I'm just thinking out loud here, but I wonder if we need to ONLY check stat type increases in the same window only. IE, you can add ATK, STA, STR, etc... in BOTH the regular buff window AND the short duration (song) window. I'll look into this more.

[PeteSampras]

.

[demonstar55]

This is on live. Plus it would be better to check spell->DurationWindow == 1 since there can be other class spells that end up in there.

[SwiftyMUSE]

Well, I think what Pete is saying is that at least one of the spells is a bard spell.

I have code that I'm currently testing that will handle this situation. I will be checking for SPAID=(2,4,5,6,7,8,9) (I'm purposely skipping CHA atm), a Beneficial Spell, and one is a regular buff and the other a short duration (song) window buff.

[PeteSampras]

.

[demonstar55]

None of those HoTs stack AFAIK, and the change I think needs to happen won't break that. I'm not sure about the necro stuff though.

[PeteSampras]

i know they dont stack, but if you remove songs from .Stacks it will say TRUE. That was my point. and every class has something similar to that issue. Ignoring songs makes .Stacks wrong.

[SwiftyMUSE]

ok im confused. Pete, are you saying that Zombieskin DOES NOT stack with Death Bloom/Blossom?

[demonstar55]

ok im confused. Pete, are you saying that Zombieskin DOES NOT stack with Death Bloom/Blossom?

I would assume it does, but it's pointless to cast Zombieskin while Death Bloom is up since it will instantly be eaten and just waste 1200 mana.