Expansion/legion

[vendethiel]

First commit contains my changes Second commit contains the libs.

I really want to ditch .pkgmeta for many reasons, most of which I explained in #1 already. .pkgmeta doesn't help with basically anything – it doesn't provide versioning, it has stale references to everything, and it encourages using a tool that no one uses. If anything I'd rather use git submodules. I also had to patch LibCooldownTracker to add per-spec cooldown overrides like legion has.

[vendethiel]

Looks like the whitespace changes (huh?) are really messing this up.

Better viewed with Github's ?w=1 option.

[slaren]

Different type of line endings maybe ?

[vendethiel]

I think so too.

[vendethiel]

I only linked to the first commit btw, the second one has the libs and is not too interesting.

The hack in LibCooldownTracker is here. Here is an example.

[vendethiel]

Uhm, btw... It seems WowAce's registration module is disabled. No clue what to do there..

EDIT: wrong, I was able to login with twitch.

[slaren]

Can you try to fix the whitespaces issue ? You obviously spent a lot of time on this and I kinda feel bad for asking this but I would rather not break the change history. Github also won't let me add a comment for a specific line in the ?w=1 pages.

The .pkgmeta stuff is good for keeping libraries automatically updated. It was present in wowace at a time where git wasn't that popular and most addons used subversion instead, but I guess nowadays git submodules may be a better option. Really, it only makes a different for the maintainer (less effort to keep the libraries updated), and since currently the addon is unmaintained, copying the libs in the repo is better than the alternative. However I would highly recommend using one of the two options (.pkgmeta or submodules) rather than just manually copying the libs to the repository, otherwise keeping them updated is going to be a major waste of time.

[vendethiel]

Can you try to fix the whitespaces issue ? You obviously spent a lot of time on this and I kinda feel bad for asking this but I would rather not break the change history. Github also won't let me add a comment for a specific line in the ?w=1 pages.

I feel the same, yeah.

[vendethiel]

Ugh, this is gonna be super annoying. I probably will remove my commits and force push a new version -_-.

[vendethiel]

Much better! Now it's all good.

I mean, I lost my own history, but I did things the wrong way, so no big deal. yay for push --force...

[vendethiel]

Just as a reminder of sorts – this is the first time I do anything major on an addon, so... Probably looks a bit gory at times.

[Rainrider]

AFAIK, you can't use gitmodules with svn repos. The ACE libraries are still on svn.

Apart from that, .pkgmeta would allow you to offer a disembedded installation option and the use of replacement tokens, which are nice for versioning. You could also generate your curse/wowace page from a file in the repo. See https://www.curseforge.com/docs/packaging

[vendethiel]

Generating a git mirror for a svn repo is very easy, however, github deals with that nicely. I'll consider keeping the Ace libs tho. I won't need to change them in the foreseeable future.

What use is there offering a disembedded installation?

[Rainrider]

You still have to update the git mirror right? It can be automated but why the hassle, when you can have the whole thing done for you by a single simple file?

Disebedded installs are useful for developers interested in memory consumption. With embedded installs the memory foot print of the lib will be blamed to the addon that loaded it first. The other two benefits of disembedding are mostly ignored by users - only one instance of the lib is installed at once (so no wasted disk space and no multiple version comparisons/loads by LibStub) and they can upgrade libs independently without you having to push a new version of your addon. Also, the normal development workflow should be disembedded.

[vendethiel]

@Rainrider You do have to update a git mirror, but adding a cron to do that would be very basic. Maybe you know if I can pick a middle-of-the-road approach, and have some libs in the repo, as well as some via .pkgmeta? I develop directly in my Interface folder (I don't mind since it's git'd anyway), so I'd have to ignore out some directories if I were to go for that – Disembedded looks like overkill for addon development. I see why it's there, but I'm not too worried about having Ace3 twice on disk.

The reason I don't want .pkgmeta is because it's just Yet Another Dependency Manager that provides no special benefit.

@slaren any other notes? If I need to make other changes I'll only be able to test them in a week when I get back. Please let me know if/when you give me access to wowace/curse LegionEx.

[Rainrider]

@vendethiel I'll just share my experience, you decide if it suits your workflow or not.

AdiButtonAuras (ABA) depends on several libraries, I'll use LibStub (LS), LibPlayerSpells-1.0 (LPS) and LibSpellbook-1.0 (LSB) for the example. My personal oUF layout also uses LS and LPS. All of them are separate addons in my Interface/AddOns folder = I run them disembedded. I maintain LPS and LSB, so I push changes to them and It makes a lot of sense to me to have only one instance of them on my drive instead of going the git submodules route. Github holds the dev repos for all of those (except for LS, which is on wowace and uses SVN). ABA and LPS are hosted (just the packages not the git repos) on wowace/curse and are auto-packaged by their system. LSB is only hosted on Github, but the curse packager can pull form every openly accessible svn/git/hg repo out there, it just needs the url and the relative path in your addon where it should extract the repo, and you list those in .pkgmeta.

I use the curse (now twitch) client for updating the addons I don't contribute to. LS is one of them. It detects addons I modified and does not auto-update them.

So, I can modify, maintain, distribute and update all of my addons in a consistent manner. All I have to do for this is edit the .toc and .pkgmeta files of my own addons, where it is two extra lines per toc file (only if you want to support disembedded installs for end-users, else you don't need to change anything there) and one line per addon dependency in the .pkgmeta (plus one extra line to disable disembedded installs if you want that).

In your case, you could just edit your current .pkgmeta (the repo urls it has are outdated) or convert from svn to git, write your cronjobs and use git submodules.

Edit: And you will still have to generate a package for distribution to users yourself if you don't use the curse packager (not that it is the only option, but still)

[vendethiel]

That's much cleaner now. thanks for the detailed explanation.

[slaren]

I agree, and seeing that git submodules aren't really an option I would be much more confortable with fixing the .pkgmeta instead of just manually inserting the libs here.

[vendethiel]

If I can keep LibDRData and LibCT in the repo while using pkgmeta for the rest, maybe. I'll definitely sound a bit ranty but...It's still mighty annoying as a developer (because I'll need to gitignore my own versions or use the @no-lib-strip hack+disembedded; and keep them up-to-date for myself) and I very very much prefer to have pinned deps rather than "svn commits are our versions" which Ace3 seems to do but whatever.

fwiw, the base Gladius discarded .pkgmeta a good while ago, and just has the libs in their github repo. They only update libs when they do want a new feature or smth broke, rather than having to pin to a svn commit or using "latest" and praying they don't break backwards compat.

Anyway, the earliest testing I can do is next week.

[slaren]

The addon should be compatible with the latest version of Ace3 anyway (same for other libs too really), since any other addon could be embedding a more recent version and it would override ours. It's fine to manually embed libs like DRData if they are no longer maintained. However I am not sure why you want to disembed LibCT, just push your changed to its repository.

For what it's worth, the way I would develop GladiusEx back in the day is with the libs disembedded. I would just let the curse client/whatever keep the the libs updated to make sure that I would always be developing with the latest versions. That worked for me, and other than once in a while having to test the automatically packaged version to make sure that embeds were working fine it had very little overhead. Certainly much less than it would have been to have to manually update every lib.

Anyway, I know it is more work to fix it now and if you don't want to do it I won't press on it. Just let me know and I'll accept the PR and give you permissions to the wowace project so that you can upload a .zip or whatever.

[slaren]

About Gladius, I don't think they ever used a .pkgmeta. I don't think they even used Ace back when I forked it. A significant amount of time was spent adapting it to my standards.

[vendethiel]

I had to add a hack to LibCT to make it gladiusex-aware, because I wanted to be able to overload cooldowns based on class/spec (Legion talents that reduce CD etc often do not create a new spellID, annoyingly). That's the reason why I want to keep it here.

If you say you were developing it disembedded, then I guess I can try. I just need to figure out how to package it locally so I can do "dry runs" and confirm the packaging works(like you said). I don't really want to use curse (memory hog and I don't trust them too much) but I guess it's better than a perl script to update all the svn repos...

Thanks for spending all that time explaining stuff, both of you :-)

[slaren]

You don't need to package it yourself, just let the wowace packager do it (it should automatically generate a packaged .zip every time a commit to master is pushed) and download it. It's fine if the packaged .zip is broken for a few versions until you get it right, that's what alphas are for anyway. Once you get it right make a tag and a beta will be generated. However as far as I know the .pkgmeta system is exclusive to wowace/curseforge so I don't think you can avoid using curse.

[slaren]

I would also prefer to keep LibCT in its own repository even if it now depends on GladiusEx. You could keep the changes in a different fork but I don't think anyone is using it anyway.

[vendethiel]

True, I could keep it in a diff. repo. Would just need to make it very explicit that it's for gladiusex only.

[Rainrider]

I only had a quick look as you don't have a clean history and it is kind of PITA to track changes, but you added one single function to LCT that depends on GladiusEx - GetCooldownTime - and it returns data even when GladiusEx is not found. And you only have one case where this function will return different data with GladiusEx - subtlety rogues using Thief's Bargain, which you always assume anyways, so why bother with this and not just set the basic cooldown to 30 sec for sub rogues? So LCT is not dependent on GladiusEx either way.

[vendethiel]

LCT doesn't have spec info, AFAIK, that's why I need this. (I also assume all discs run with fear CD talent)

[Rainrider]

I see https://github.com/slaren/LibCooldownTracker-10/blob/master/cooldowns_hunter.lua#L175-L180 in the old version, so I assumed it has it.

The only use of cooldown_overload in your fork is in the rogue data of LCT. Maybe you haven't uploaded it yet. But even then, it is the same use case as for sub rogues - you assume it is always used, because you can't know the talents of your pvp opponents.

Btw LCT data layout could be improved. Listing the class in every spell info table is a waste. Why not organize by class, spec, category? You could also move from associative to indexed arrays too (the categories might be a problem here though, but it can be solved by bitmasks), You could also have duplicates there, so if you load spec data after you have loaded the spells shared by all class specs, you could overwrite the shared spell with the spec specific one without introducing new fields like cooldown_overload.

[vendethiel]

The reason I have to use the _overload is because the arrays are indexed by spellid. Sub rogue's vanish and other spec's vanish share spellid, but not thief's bargain.

That doesn't solve the issue at hand tho. I still need to know, from libct, what's the player's spec.

I agree the layout could be improved, but I don't want to get into that yet, and not in this PR.

[vendethiel]

When I say "LCT doesn't have spec info", I mean it doesn't know the spec of whoever casted the spell. That's the interesting part. The specID is just used for display purposes in gladiusex.

[slaren]

The spells are indexed by spellid mainly because it is the way they are looked up in the places where performance is a concern (in event handlers). I guess you could still input the data in a different format and then generate a lookup table on startup, but personally I didn't find having to type the class/specID in every entry to be a big enough annoyance to do anything about it. Also keep in mind that it supports cooldowns that are not tied to a class, like racials and items.

Without giving it a lot of though, I think the cleanest way to solve the dependency issue would be just to move all the spec detection code from GladiusEx to LCT. Then some of the logic that is currently in the cooldowns module could also be moved to the LCT, making the library easier to use and the cooldowns module code cleaner.

[vendethiel]

That's probably a good idea, but not something I'm willing to do in this PR, tbh.

[vendethiel]

Btw we already generate lookup tables by class/spec for the iterator at startup. The layout will most probably stay the same.

[Rainrider]

My point of interest here was primarily why you introduce a circular dependency between GladiusEx and LCT.

You could take a look at the cooldowns module of oRA3 if you wish to improve LCT.

[slaren]

There is a big difference between detecting cooldowns of opponent players than of those in your own party. oRA3 is great but it is just not comparable.

[vendethiel]

My point of interest here was primarily why you introduce a circular dependency between GladiusEx and LCT.

I know I do. The only other option is replacing the function (i.e. providing LCT:SetSpecGetter or whateverelse I could imagine), and it's not pretty either.

[vendethiel]

I tried to do that, but it just breaks everything, docs on the subject are non-existent, trying to follow what other repos do or reading LibStub docs is proving very annoying.

It works except for LibFunctional, somehow, so clearly it's me missing something.

[vendethiel]

Okay, I actually managed to make it work.

Thanks for your help, re-reading all these comments helped me tremendously :-).

I need to fork LibCT and DRData and I'm done.

[vendethiel]

Pushed the cleanup.

[slaren]

I also noticed that you have copied the localization data to the repository. I don't mind if you want to stop using it, but before it used the wowace localization tool, so that anyone could contribute translations.

[vendethiel]

It's less about downloading and more about "i started off the version from wowace"

[vendethiel]

Fixed the toc. How does the localization tool work?

[slaren]

You just have to add the strings that need to be localized to the localization tool, and then anyone can translate it to their language using the web app. The packager then automatically generates the translated locale tables.

[vendethiel]

Should be good to merge now!

[slaren]

Looks good to me, merge it whenever you want. Do you have a wowace/curseforge/curse/twitch/whatever it is called now username that I can add to the wowace project ?

[vendethiel]

yeah. Vendethiel.

[vendethiel]

🍰 🍰 🍰 🍰

[slaren]

Are you able to see the status of the packager at https://www.wowace.com/dashboard/builds ? The build failed with this error: Invalid pkgmeta file: [Line: 24, Col: 12] While scanning a simple key, could not find expected ':'.

[vendethiel]

should be fixed

[slaren]

Failed again: Invalid pkgmeta file: [Line: 23, Col: 5] Duplicate key

[vendethiel]

Ok. Should be fixed too.

Btw, I plan to declare "issue bankruptcy" and start anew, that is, close all issues open atm.