Ralgathor / LibClassicSwingTimerAPI

GNU General Public License v3.0
4 stars 5 forks source link

Prepare all active version support #29

Closed Ralgathor closed 1 year ago

Ralgathor commented 2 years ago

@mrbuds @hypernormalisation if you have time I would like a review from you on this changes to allow support for all active version of the game.

You can ignore the "Split table data on different files by version" commit as I have reverted this change to keep the lib to one single file.

Ralgathor commented 2 years ago

thx @mrbuds for the reviews! I have tried something "different" for the spells table to reduce the amount of table lookup. My goal it's to maintain the lib in one unique file to be more easly embeded in another addons.

Optimise spells table lookup to access value.

mrbuds commented 2 years ago

My goal it's to maintain the lib in one unique file to be more easly embeded in another addons

Indeed i was baited by toc files and forget addons use xml .. but you could make a xml file per game version and it's up to addon to load correct xml file

Otherwise if you keep it in one file i think it's easier and more efficient to do this

local list_reset_swing_spells
local reset_swing_channel_spells
local prevent_swing_speed_update
if isClassic then
  list_reset_swing_spells = {
    ...
  }
  reset_swing_channel_spells = {
    ...
  }
  prevent_swing_speed_update= {
    ...
  }
elseif IsBCC then
  list_reset_swing_spells = {
    ...
  }
  reset_swing_channel_spells = {
    ...
  }
  prevent_swing_speed_update= {
    ...
  }
elseif IsWrath then

elseif IsRetail then

end

Current way memory hold tables with data for all versions, use them only 1 time, then make more tables Yes there will be duplicate lines but it's better have duplicates lines in code than unused data in memory

Ralgathor commented 2 years ago

Current way memory hold tables with data for all versions, use them only 1 time, then make more tables Yes there will be duplicate lines but it's better have duplicates lines in code than unused data in memory

I see the issue to having table that are only used one time. Can we make this tables garbage collected? I read some stuff one Lua memory usage and it seems if I set this tables local ref to nil the table should normally be garbage collected.

I'm really not fan of maintaining multiple tables by game version. It's seem very messy for me and I prefer the current format.

Ralgathor commented 2 years ago

I see the issue to having table that are only used one time. Can we make this tables garbage collected? I read some stuff one Lua memory usage and it seems if I set this tables local ref to nil the table should normally be garbage collected.

I'm really not fan of maintaining multiple tables by game version. It's seem very messy for me and I prefer the current format.

Nevermind, I read more about Lua code perf and it's better to create table with already define size that doing this dynamic table construct. I think that I need to accept for performance to have a more messy code to maintain!