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

enable CainIdentify with CharacterCfg option #569

Merged
merged 8 commits into from
Dec 19, 2024

Conversation

srliao
Copy link
Contributor

@srliao srliao commented Dec 13, 2024

Added option to enable the existing CainIdentify code from here. Tested on SP and it seems to work pretty well.

@srliao srliao marked this pull request as draft December 13, 2024 13:50
@srliao srliao force-pushed the enable-identify-with-cain branch from e6c2c66 to 99e8988 Compare December 13, 2024 13:55
@srliao
Copy link
Contributor Author

srliao commented Dec 13, 2024

Added option to UI and tested to make sure it saves to the config file

{58319C05-6801-46F5-81BD-29A00B48678E}

@srliao srliao marked this pull request as ready for review December 13, 2024 14:39
@artosimonyan
Copy link
Collaborator

Hehe, forgot to enable this as I didn't test it extensively after adding it. Glad to hear it's working as expected.

Few notes though:

  • Wording for the UI option is bad. Change it to Identify with Cain, omit the long explanation afterward.
  • Move the UI element above the minimum gold option
  • Most important: We need fallback logic in case identifying with Cain fails. For example if Cain wasn't found (ie wasn't rescued), it should fallback to normal way of identifying.

@srliao
Copy link
Contributor Author

srliao commented Dec 14, 2024

Hehe, forgot to enable this as I didn't test it extensively after adding it. Glad to hear it's working as expected.

Few notes though:

* Wording for the UI option is bad. Change it to Identify with Cain, omit the long explanation afterward.

* Move the UI element above the minimum gold option

* Most important: We need fallback logic in case identifying with Cain fails. For example if Cain wasn't found (ie wasn't rescued), it should fallback to normal way of identifying.

Thanks for the comments. I'll get the first two points fixed. Regarding the fallback logic, we'll need this PR merged first unfortunately: hectorgimenez/d2go#54 as currently the quest data is not present.

In addition, the UpdateQuestLog function is currently bugged because

for ctx.Data.OpenMenus.IsMenuOpen() {
does not actually check if QuestMenu is open. Related discussion here: https://discord.com/channels/1163042152985661480/1316961789191389254/1317256132594765897. I'll submit a separate PR to fix this

@srliao srliao force-pushed the enable-identify-with-cain branch from 1a13664 to 09ac51a Compare December 14, 2024 15:40
@srliao
Copy link
Contributor Author

srliao commented Dec 14, 2024

Updated as requested:

{C161CF93-C8A0-42E3-A5F7-C8066FD4B9FE}

@artosimonyan
Copy link
Collaborator

Revert this: 09ac51a.

We don't need anything from d2go. If it doesn't find Cain in town, the Identify with Cain will return an error, in which case we can proceed and use old logic.

Don't try to overly complicate stuff.

@srliao
Copy link
Contributor Author

srliao commented Dec 14, 2024

Revert this: 09ac51a.

We don't need anything from d2go. If it doesn't find Cain in town, the Identify with Cain will return an error, in which case we can proceed and use old logic.

Don't try to overly complicate stuff.

Done

@IronMaXxX
Copy link

Maybe switch line 119 and 120 in character_settings.gohtml, so the checkbox for "Use teleport when available" is also in front of the text like all other checkboxes.

grafik

grafik

@srliao
Copy link
Contributor Author

srliao commented Dec 19, 2024

Maybe switch line 119 and 120 in character_settings.gohtml, so the checkbox for "Use teleport when available" is also in front of the text like all other checkboxes.

grafik

grafik

I assume that should be a separate PR since it's an unrelated change to Cain? I can make the change on this PR if the maintainer wants me to though

@artosimonyan artosimonyan merged commit 02b6aa1 into hectorgimenez:main Dec 19, 2024
1 check passed
@srliao srliao deleted the enable-identify-with-cain branch December 20, 2024 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants