Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New save format #7050

Merged
merged 2 commits into from
Dec 17, 2023
Merged

Conversation

asvanberg
Copy link
Contributor

Uses the Path of Exile BaseItemType.Id for the gemId save format attribute in the <Gem> element as it always has been. The skillId attribute is also kept and maps to the gem's variant primary GrantedEffect.Id. Adds a new attribute called variantId that maps to the variant of the gem (GemVariants[n].Id), eg. Arc, ArcAltX, or ArcAltY. This is to help identify which variant is used rather than relying on every gem variant having its own unique granted effect.

During loading if there is no (known) gemId or variantId it falls back to looking at skillId and eventually by name, as before.

For the a lot of gems skillId and variantId are the same but it is not true for every case, especially support gems.

Hopefully by relying on the game's identifiers rather than the internal key of the Data/Gems.lua table we are both more free to refactor the internals of the application as well as being more aligned with how the game identifies gems and their variants. It should also make it easier for 3rd party developers to understand which skill gems are used.

Uses the Path of Exile BaseItemType.Id for the gem in gemId (as it always has been).
The skillId attribute is also kept and maps to the gems variant primary GrantedEffect.Id.
Adds a new attribute called variantId that maps to the variant of the game, eg. Arc, ArcAltX, or ArcAltY. This is to help identify which variant is used rather than relying on every gem variant having its own unique granted effect.

For the majority of gems skillId and variantId are the same, but it is not true for every case.
Copy link
Member

@Wires77 Wires77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding the variantId to the save file seems redundant given the info we already have in the save file. Otherwise this looks good.

src/Classes/SkillsTab.lua Outdated Show resolved Hide resolved
local function setupGem(gem, gemId)
gem.id = gemId
gem.grantedEffect = data.skills[gem.grantedEffectId]
data.gemForSkill[gem.grantedEffect] = gemId
data.gemsByGameId[gem.gameId] = data.gemsByGameId[gem.gameId] or {}
data.gemsByGameId[gem.gameId][gem.variantId] = gem
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just make this map's inner key be grantedEffectId? That way we don't need to add variantId to the save file format. Then in SkillsTab.lua you can skip the if statement and loop and do possibleVariants[child.attrib.skillId]

Copy link
Contributor Author

@asvanberg asvanberg Dec 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I chose to use the variant id over the granted effect was that it better mapped to what we actually wanted to identify, the variant of the gem. While right now the primary granted effect is unique and does work as well, it may not always be the case. For example, they can be (accidentally) swapped so that what is now GrantedEffect could become GrantedEffect2 and vice versa or if GGG decides to add "Arc of Combination" that grants both Arc and ArcAltX or similar.

All in all it just felt like the safer and most future-proof way to identify which gem was being used and since it is very desirable to support old save formats, picking the one least likely to change seemed like the path forward.

There is also the point of third party tools which parse and use our save format. If we could get some feedback from them it could help nail down the best format to use.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well reasoned, and if we're going to change our format, adding the variant explicitly seems like a solid way forward. I'll throw the question in discord to the other third party tool devs about format

@LocalIdentity LocalIdentity added the enhancement New feature, calculation, or mod label Dec 17, 2023
@LocalIdentity LocalIdentity merged commit f754f69 into PathOfBuildingCommunity:dev Dec 17, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, calculation, or mod
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants