This repository has been archived by the owner on Jul 1, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 381
Walking Hammerdin bugfix #749
Closed
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this function removed? I'm not 100% sure of the consequences of this but @VladimirMakaev might know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering the same which is why I asked if it still works for hdin with tele. Ive been running as a walkadin and seems fine. Ill double verify in a bit if enigma still works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When debugging self._do_pre_move it turned out that it reverts back to true. The Hammerdin class logic expected it to stay false though. It was the main culprit of why the Hammerdin seemed to "attack" while having Vigor turned on. It should have switched to Concentration, but didn't. Not relying on self._do_pre_move fixed it immediately and then the on_capabilities_discovered wasn't required anymore.
Also it lead to unneccesarily complicated logic constructs that were hard to follow and could be done in a simpler and more readable way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't touch this code. It can easily break something for Enigma / Tele-Charge paladin . And you need to test 3 different version of paladin on all routes to make sure it's sound:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should just cast Concentration and move with PreMove=False that way you won't change to Vigor.
preMove() will flip to Vigor in case of Walkadin or Tele-Charge (there is no way to pass that telecharge is to be used based on current code structure). So in case you're opting to tele-charge then you should manage PreMove part manually. E.g. casting concentration and doing PreMove=False, TeleCharge = True or PreMove=True if you want Vigor to be on for walking / running out of tele charges and walking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It didn't even work the way it was intended to work. No need to re-introduce bugs.
This PR has been tested extensively with all three variants. Some people even happily and successfully use it for their daily runs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which is what I did.