Add support for skill sets (socket group sets)

[deathbeam]

Adds support for skill sets for socket groups. This is useful for adding
different sets for different character state or progression (e.g
Acts/Starter/High Budget etc).

Fixes #2060

Signed-off-by: Tomas Slusny slusnucky@gmail.com

Example:

"Arc set"

"Crackling lance set"

The build values/reservations and active skill is properly updated.

Example pob:

https://pastebin.com/u6zGbcp0

The format the data is stored in:

[deathbeam]

Added also missing copy function for sets and fixed up changing of order

[Wires77]

I'm glad someone finally got around to doing this! I do think it should follow the same data format that ItemSets and TreeSpecs do (i.e. <Skills activeSkillSet="1"><SkillSet id="1">) We'll just have to let our build xml consumers know we're making this change and have backward compatibility with the old format

[deathbeam]

Ah alright, that makes stuff a lot easier, I was trying to keep the format as close to current as possible

[deathbeam]

Alright updated, also updated the example pob. With this ideal format the solution is a lot faster and simpler

[InevitableWaste]

I'm not sure what your deprecation policy is, but you may want to consider a 1 release deprecation. Keep a backwards compatible format (perhaps by appending all the sets together but keeping them disabled, and using a single gem group as a "comment" in between each), and also include the new format. That will give more time for tools to update, and then in the following release you can remove the backwards compatible format.

Of course that means tracking more things and making more code changes, and nobody's got time for that.

[Wires77]

While we don't have an official deprecation policy, we do reach out to other tool developers (usually via Discord) that we know consume build files to let them know about upcoming changes that may break their workflow. The downside of doing this is that we can only contact those people who have joined Discord or that we know about. I don't think GitHub has an ability to get notifications on a certain label for PRs, because otherwise labeling things as a breaking change would be the most convenient for everyone.

As to the actual 1 release deprecation, I'm willing to give it a shot, but I'm afraid your last sentence is going to become a big factor. Perhaps making two PRs at once, one for the change, and one to merge before the next major release would help keep things fresh in mind and not forgotten.

[InevitableWaste]

That is a good idea about the double PR.

the reason for a 1 release deprecation would be to add a DEPRECATED FOR TOOL DEVS section in the patch notes, which I imagine reaches more tool devs then the discord? It would also give them more time, without being unreasonable bloat to carry forward.

I was thinking more on this, and you might be able to accomplish most this without additional PRs and code. Depending on your branching model, you could just release a NOOP release with only patch notes to notify tool devs of an upcoming breaking change they might need to be aware of. The downside of this is more releases, and nagging non-tool devs with an update/notes that don't include anything for them.

Anyways I'm just happy this feature was merged, been looking forward to it! I'll let y'all figure out if you want to do anything about the BCs or not, anything you choose including nothing seems completely reasonable approach to me!

Keep it up.