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.

Sign up for GitHub

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

Jump to bottom

Add feature to guess the main socket group when importing a character #3899

Merged
merged 2 commits into from
Jan 20, 2022
Merged

Add feature to guess the main socket group when importing a character #3899

merged 2 commits into from
Jan 20, 2022

Conversation

raylu
Copy link
Contributor

@raylu raylu commented Jan 17, 2022 •
edited
Loading

Description of the problem being solved:

Importing a character for the first time sets the main skill to the first one. This instead sets the main socket group to the one with the most gems (only when importing a character onto a build with no skills).

Steps taken to verify a working solution:

  • Imported a character and confirmed a reasonable skill got chosen
  • Imported from a code and confirmed the saved skill (which was not the largest socket group) was restored

Before screenshot:

image

After screenshot:

image

It looks like there's a bug since the CoC triggering skill isn't set correctly, but, AFAICT, that's an existing issue on dev at 6a356a5 ( but not on 2.13.0; see #3900)

@raylu raylu changed the base branch from master to dev January 17, 2022 03:16
@raylu
Pick main socket group when importing a character
6ce499f 
Instead of leaving the "Main Skill" dropdown at the first socket group,
try to guess which skill is the main one.
@LocalIdentity LocalIdentity added the enhancement New feature, calculation, or mod label Jan 20, 2022
Wires77
Wires77 requested changes Jan 20, 2022
View reviewed changes
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.

Some style changes and an easier check for no main skill, but otherwise looks good

@raylu raylu requested a review from Wires77 January 20, 2022 22:06
@raylu raylu closed this Jan 20, 2022
@raylu raylu deleted the pick_main branch January 20, 2022 23:23
@raylu raylu restored the pick_main branch January 20, 2022 23:26
@raylu raylu reopened this Jan 20, 2022
@Wires77 Wires77 changed the title Pick main socket group when importing a character Add feature to guess the main socket group when importing a character Jan 20, 2022
@Wires77 Wires77 merged commit 3b01560 into PathOfBuildingCommunity:dev Jan 20, 2022
@raylu raylu deleted the pick_main branch January 20, 2022 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wires77 Wires77 Wires77 approved these changes

Assignees
No one assigned
Labels
enhancement New feature, calculation, or mod
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@raylu @Wires77 @LocalIdentity