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

Attempt to fix hooks using first keyframe at first step of second sampler #6185

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

catboxanon
Copy link
Contributor

@catboxanon catboxanon commented Dec 23, 2024

Attempts to resolve #6183

This fixes the issue where hooks incorrectly use the first keyframe during the first step of a second sampler pass as described in the original issue's workflow. Workflows that sample through all sigmas are not affected by this change.

Unfortunately, I don't believe this resolves the issue entirely, as it still has to invoke the ModelPatcher at the first step of the second sampler pass when it should not be necessary, and while the results are much closer to being identical than before, it still is ever so slightly off. Regardless, I'm opening this PR for further dissection and maybe as a temporary fix until it is properly resolved.

Result of this PR (compare to result 1 in the aforementioned issue)

ComfyUI_temp_oluux_00001_

Still invokes the ModelPatcher when it shouldn't need to.
Causes very slight difference in results, but not nearly as much.
@Kosinkadink
Copy link
Collaborator

The proper 'fix' for guarantee_steps would require that the keyframe logic knows about all the sigmas that the current sampler will experience. Technically guarantee_steps=0 by default would make this problem go away, but then introduces not respecting the start strength when more keyframes exist than steps.

I should be able to make guarantee_steps work as intended with sampling split between multiple KSamplers if I modify the code a bit so the keyframe code knows all the sigmas it will work with. I'll include it with the changes for next ModelPatcher PR I am working on that fleshes out the hooks system. I should have that PR ready by the end of the week, I will refer back to this PR to let you know when it's ready.

@catboxanon
Copy link
Contributor Author

I should have that PR ready by the end of the week, I will refer back to this PR to let you know when it's ready.

Cheers. I'll choose to leave this PR open for now just for awareness but will close it when that PR is ready.

@catboxanon catboxanon marked this pull request as draft December 23, 2024 19:06
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.

Hooks disabled at first step of a second SamplerCustomAdvanced node
2 participants