Server
Server copied to clipboard
EQEmu
Consider changing spell_id from int16 to int or int32
Given the number of spells in current clients we're running up against int16 limits (esp when cast to a signed int).
We should at some point consider going through and changing all places we store spell_id to int or int32.
I think we just need to go through and switch all the remaining over to int32 now. I do highly recommend int32 over uint32 for this though.
But that limits us to 2 billion spells instead of 4 billion! How could we gimp ourselves so badly?
While I was -clearly- joking, to clarify on the recommendation for int32 instead of uint32, EQ treats SpellID as a signed number in some places, such as spell effects on items, which are -1 if there is no effect in that slot.
You weren't joking, you were bloody serious
On Sat, Nov 14, 2015 at 4:34 PM, Jon D. Jackson [email protected] wrote:
While I was -clearly- joking, to clarify on the recommendation for int32 instead of uint32, EQ treats SpellID as a signed number in some places, such as spell effects on items, which are -1 if there is no effect in that slot.
— Reply to this email directly or view it on GitHub https://github.com/EQEmu/Server/issues/7#issuecomment-156751797.
This shouldn't be closed. We still need to move spell IDs all over to a bigger sized int.
Then someone should change the datatypes quick cause it should be a fairly simple change and test so we can close this out
If it was so simple, it would be done already. I'm 99% sure I fixed one of the issues I ran into originally, but I just haven't revisted it yet.
I looked into some more issues that might arise from fixing this. It looks like even though spell IDs were switched to ints at some point, the clients still used int16_t in the BeginCast_Struct. Luckily, these clients don't support spell IDs beyond the int16_t range, so we can just handle this in translators. (RoF+, at least for us use an int for this packet)
The CombatDamage_Struct as well as Action_Struct also still use int16_t for spell ID. We'll have to handle this in the translator as well.
Not sure what we should do since this packet has a more significant client side affect ...
The CombatDamage_Struct as well as Action_Struct also still use int16_t for spell ID. We'll have to handle this in the translator as well.
Not sure what we should do since this packet has a more significant client side affect ...
Was changed to int32 in ROF1.
Clients prior to ROF1 do not support that high of spell values anyway. In the encoder, they should simply be set to unknown spell ID. (-1, or SPELL_UNKNOWN macro - though, we should probably really have the SPELL_UNKNOWN macro reference the int32_t version and use the limits for each expansion)
In fact, I'd go as far as to say we can change the code in npc spells and other related places to int32_t once that is taken care of.
I have this functionality already (mostly) in classless, will have to port it over soon(ish) - the DB should already support int, though I will have to get PEQ and confirm.
References:
https://github.com/EQEmu/Server/blob/master/common/patches/uf_structs.h#L1263 https://github.com/EQEmu/Server/blob/master/common/patches/rof_structs.h#L1461
