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 param info workflow #65

Draft
wants to merge 22 commits into
base: dev
Choose a base branch
from
Draft

Conversation

mschuettlerTNG
Copy link
Collaborator

Description:

Please provide a brief description of the changes made in this pull request.

Related Issue:

If this pull request is related to any existing issue, please mention the issue number here.

Changes Made:

Please list down the specific changes made in this pull request.

Testing Done:

Please describe the testing that has been done to ensure the changes made in this pull request are functioning as expected.

Screenshots:

If applicable, please provide screenshots or GIFs or videos to visually demonstrate the changes made.

Checklist:

  • I have tested the changes locally.
  • I have self-reviewed the code changes.
  • I have updated the documentation, if necessary.

@julianbollig julianbollig force-pushed the enableParamInfoWorkflow branch 2 times, most recently from 17af8fc to dff6c53 Compare February 12, 2025 11:09
}
imageGeneration.processing = false
imageGeneration.stopping = false
}
}

function createInfoParamTable(infoParams: ImageInfoParams) {
const mappingKeyToTranslatable: StringKV = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need for this to be in the function

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, the mapping to translation keys is display logic and should probably reside in the component that displays this.

Choose a reason for hiding this comment

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

For reusage of infoTable, I wanted to keep it clean from backend specific mappings. Also, how it is now, the information stored in the generated images is less and cleaner. Further, in case we want at some point to restore images or recreate the settings using the information on the table, it might be easier, if it is more structured from the start.

lora: infoParams.lora,
safe_check: infoParams.safe_check,
}
return Object.fromEntries(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If these two objects should map to each other, this should be reflected in the type.

Choose a reason for hiding this comment

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

I am not sure what you mean by that, can you elaborate further?

@julianbollig
Copy link

The way the image preview is designed now, it always jumps to the last image whenever the number of generated images changes. If you delete an image, that might not be, what you want. Shall we leave it that way or account for this special case?

…bjects rather than just urls

Signed-off-by: julianbollig <[email protected]>
…allow for infoParam in ComfyUi

Signed-off-by: julianbollig <[email protected]>
Signed-off-by: julianbollig <[email protected]>
Signed-off-by: julianbollig <[email protected]>
Signed-off-by: julianbollig <[email protected]>
Signed-off-by: julianbollig <[email protected]>
Signed-off-by: julianbollig <[email protected]>
@julianbollig julianbollig force-pushed the enableParamInfoWorkflow branch from 0bb3050 to 6c5a77a Compare February 17, 2025 20:08
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.

2 participants