Not All AA Displayed

[Cilraaz]

I might have just found the issue (though I'm SUPER rusty with C++ and terrible with bitmasks in general). In zone/aa.cpp, on line 1421, there is the following code:

        if(IsClient()) {
                if(!(CastToClient()->GetPP().expansions & (1 << rank->expansion))) {
                        return false;
                }
        } else {
                if(!(RuleI(World, ExpansionSettings) & (1 << rank->expansion))) {
                        return false;
                }
        }

If I'm reading this right, it's checking the expansion value (forced by World:ExpansionSettings in the rule_values table) against the expansion for the AA. If we use the example that I'd originally quoted, Ferocity, that expansion value is 4. So the bitwise left would be "1 << 4", which is 1*4^2 or 16. That would evaluate "0" after the bitwise & against an expansions variable of 15. As such, shouldn't the code be:

if(!(CastToClient()->GetPP().expansions & (1 << (rank->expansion - 1)))) {

This would be 1 << (4-1) or 1*3^2, which is 9, which would evaluate "9" after the bitwise & against an expansions value of 15.

I made this change and recompiled zone. The AA's showed up properly (SoL and PoP together) when using an ExpansionSettings value of 15. I changed it to 8, restarted the server, and only PoP AA showed up, as expected.

Could a dev take a look and see if I'm correct here or let me know if I'm a complete idiot?

[Cilraaz]

After today's update to eqemu_update.pl, I tried updating my AA tables via the script and recompiling zone without the above modification. They again showed an expansion behind. I was hoping maybe my AA table having the old data was the problem, but apparently not.

Any thoughts on the above code modification being right or completely wrong?

[Shendare]

Guess there aren't a lot of people working on expansion limiting AAs after the revamp to chime in. :/

[AdrianD]

I'm definitely interested and would help if I could.

[Noport]

you might have to bush up on your c++ programing and work with eq_packet_structs

[AdrianD]

Originally Posted by Noport

you might have to bush up on your c++ programing and work with eq_packet_structs

There's no brushing up possible. It's strictly learning from the ground up. =P

[Cilraaz]

So I took a look at this again today and I think I was right above, but for the wrong reason.

I looked into bit shifting a bit more and realized I was calculating it wrong above (though still coming up with kinda the right answer). I think my logic needs to look at what possibilities should evaluate true. I'm going to do a bit of rambling just to get thoughts down into the thread. Feel free to pick any of it apart.

Earlier, AdrianD posted the expansion value chart:

Expansion Value	All Expansions	
0	Original0	Original
1	Kunark	1	Kunark
2	Velious	3	Velious
4	Luclin	7	Luclin
8	PoP	15	PoP
16	Ykesha	31	Ykesha
32	LDoN	63	LDoN
64	GoD	127	GoD
128	OoW	255	OoW
256	DoN	511	DoN
512	DoD	1023	DoD
1024	PoR	2047	PoR
2048	TSS	4095	TSS
4096	TBS	8191	TBS
8192	SoF	16383	SoF
16384	SoD	32767	SoD
32768	UF	65535	UF
65536	HoT	131071	HoT
131072	VoA	262143	VoA
262144	RoF	524287	RoF
524288	CoF	1048575	CoF
1048576	tDS	2097151	tDS

If we convert that to the bitmask values, we get (inclusively, it would be all bits after the initial 1 also being 1 instead of 0, but that wouldn't really change anything):

SoL  - 00000100
PoP  - 00001000
LoY  - 00010000
LDoN - 00100000
GoD  - 01000000
etc

Now, if we look at bitshifted values of expansions from the AA table (results after the "1 << rank->expansion" calculation), we have:

SoL  - 00001000
PoP  - 00010000
LoY  - 00100000
LDoN - 01000000
GoD  - 10000000
etc

So in the code now, isn't it comparing SoL (0100, or inclusively 0111) to the bitshifted AA expansion variable for SoL (1000)? If so, then wouldn't this always evaluate false, making the code correction posted earlier correct?

if(!(CastToClient()->GetPP().expansions & (1 << (rank->expansion - 1)))) {

Again, I'd kind of like a dev (or anyone who knows this code, C++, bitmasks, bitshifting, etc better than I do) to take a peek and let me know if I'm insane or not.

[Uleat]

I haven't been ignoring this one..my online time is limited and I've attempted twice to respond to it - without luck...


You can set bitmasks up to represent just about anything..but, looking specifically at the expansion value chart posted (re-posted) above:

Kunark has a bit value of 1 and a bitmask of 1.


A standard ordinal bitmask will usually exclude 0 from its rankings due to the fact that the lowest provable ranking value is 1 - there is no 0's column
in bit representation.

To convert an ordinal bit ranking to bit position, yes, a simple (1 << (ord_val - 1)) should be used. For cases of 0, you would simple prove it by
checking (bool)ord_val == false or ord_val == 0.

SIDENOTE: You can create a positive mask for validating greater than the ordinal ranking by using (~0 << ord_val). C++ doesn't implement bit rotations,
so there is no chance of a wrap-around with a standard bit shift.


In the case of Kunark, converting the 'value' to a bit position in the coded example does indeed produce 0x02 where the mask is 0x01.

However, look at what is being considered: CanUseAlternateAdvancementRank

Since the transformation reflects a greater than positional value, and there is a 'false' return methodology, you really don't want that condition
to be true for an exclusionary check.


There's a lot of systems that I'm not up-to-speed with - this being one of them.

But, looking at what is defined and what is being checked, I think that particular code snippet is correct. That's not to say that there may not be
issues elsewhere (these guys are pretty sharp and usually catch logic errors pretty quick.)

[Cilraaz]

Originally Posted by Uleat

Kunark has a bit value of 1 and a bitmask of 1.

Kunark would be the oddity. I hadn't considered it, since Kunark didn't have any AA availability.

Originally Posted by Uleat

However, look at what is being considered: CanUseAlternateAdvancementRank

Since the transformation reflects a greater than positional value, and there is a 'false' return methodology, you really don't want that condition
to be true for an exclusionary check.

Perhaps I'm misunderstanding your intended message, but since it returns false on a not true, the result from the bitwise & should be true if the AA is to be visible to the player. Using SoL as an example, since I can break the comparison down to just a nibble, we currently have the bitwise & of the expansion bitmask (0100) and the shifted rank->expansion value (1000):

0100
1000
----
0000

Throwing a not on that then evaluates true and in turn returns false, even though we obviously want SoL AA to be used if SoL is the enabled expansion

Originally Posted by Uleat

There's a lot of systems that I'm not up-to-speed with - this being one of them.

But, looking at what is defined and what is being checked, I think that particular code snippet is correct. That's not to say that there may not be
issues elsewhere (these guys are pretty sharp and usually catch logic errors pretty quick.)

Which snippet do you believe is correct? The one I posted or the one in source. Perhaps the above bit in this reply is wholly unnecessary.

[Uleat]

[Still think I'm missing something here...]


Let me ask around on this one..it's very possible that we need to adjust some things.

[Cilraaz]

Originally Posted by Uleat

No, looks like you're correct.

I didn't catch the bool flip when I was scanning over the function today...


Let me ask around on this one..it's very possible that we need to adjust some things.

From my limited testing and logically, it should work. I'm not intimately knowledgeable of the source, though, so I wasn't sure if anything else needed modified accordingly.

Thanks for taking a look!