Inconsequent naming of new API node SaveSVG breaks other nodes #7980
Comments
|
Hey, thanks for raising this. Sorry about the conflict, I was not aware there was a custom node that already used the "SaveSVG" id; for custom nodes, it is ideal that they always include a prefix or postfix on node ID's to avoid any clashes with other nodes - "SaveSVG" is a very general name and likely would experience conflicts with other custom nodes that may have a similar feature. There was a very short turnaround time for getting SVG implemented in code. The Save SVG node is in the recraft folder and does not have a recraft prefix because I intended it to be the start of official ComfyUI SVG support, and after the launch settles down I would then move the node out of Recraft-related API node code into a more general place. This made development less likely to run into conflicts with other core code for merging purposes. The Save SVG node also adds metadata to the SVG output to allow it the workflow to be reproduced by dragging the .svg into ComfyUI. Since the nodes are out and distributed on templates in the UI, etc., I don't think the core ID can be changed anymore, as that itself will cause more issues. I took a look at the code to see if there would be an easy way to moe forward, and a common thread is that SVG type is class that stores a list of BytesIO objects (file-like), and the custom node passes a STRING that is actually a list of strings that are the binary representations of the .svg files. The use of STRING type is unfortunate, as there is no way in code to determine when it is 'not' a string; something that returns a list of strings might be another custom node trying to do something non-SVG related. ComfyUI supports multiple types for an input by doing "Type1,Type2", so if that were the case, I could have added that type as an additional allowed input for the core ComfyUI node and made the code changes to accommodate differences in the byte data format. Not ideal, but I think the best way to move forward would be for the custom nodes that return STRING lists and take the STRING lists to be deprecated (you can add DEPRECATED = True to the class definitions) and replaced by new nodes with new IDs but identical names, and that would use the SVG classes and inputs. I can make a PR to core to move all the SVG helpers/classes into a general location so you can reference it directly there. |
|
Or, alternatively, only delete your SaveSVG/PreviewSVG nodes, and add a helper node to transform your string outputs into SVG outputs. |
|
If you want to avoid having people on older versions of ComfyUI getting your version of Save SVG node removed (since it does not exist in core), you can add some code to determine if version of ComfyUI is new enough, and then have an if/else that changes how the NODE_CLASS_MAPPINGS dictionary gets populated. |
|
Thanks for your swift and detailed reply. @Kosinkadink ; On the ComfyUI part, it would then also be best to move the SaveSVG out of the API nodes soon, as to not have the node disabled by the |
|
There's actually a PR submitted that hasn't been merged in yet that will do just that! |
|
Then i will hurry up with my fix :) |
|
@Kosinkadink fix and temp fix on my node is live, updated Yanick112 with info how to fix his. I opted to have the old route actively show 'deprecated' for people to see they will have to use the new route in a newer version. And feel free to install the node and try it out ;) |
|
Sorry for the delay, here is the PR that will move the core SVG stuff out of recraft code: #7982 |
|
Issue can be closed; all working great now; also after the update that pulled SVG out of recraft and into core |
Expected Behavior
The latest update (ComfyUI v0.3.32) has additions to API nodes with PR #7956 by @Kosinkadink
Every node added has the naming to the system it is linking
Except Recraft SaveSVG
(bottom item)
As the functionality in the SaveSVG node is specific to the Recraft API (and only works with the API), the expected behavior is to have it named specifically linked to the active API: Recraft SaveSVG , both in naming as in properties.
Actual Behavior
The effect of not being named consistently is it breaking existing SaveSVG nodes (ComfyUI-ToSVG) and and creating confusion for endusers using ComfyUI-ToSVG and (my) ComfyUI-ToSVG-Potracer nodes and enduser workflows.
Yes, they are custom nodes, so a reasoning could be to have it resolved on that end, but in my opinion, consistently naming items should prevail. Especially since the Recraft SaveSVG functionality does not work without the API.
Therefor, the 'generic' name SaveSVG shouldnt have been used in the first place.
Should the SaveSVG node be updated to a generic base ComfyUI node, working without API and outside of the API Nodes tab, there would still be a conflict, but then it would be on the custom node to resolve it.
Steps to Reproduce
Properties panel of (every other) API node has the API in the naming
Except the SaveSVG API node
Debug Logs
-Other
No response
The text was updated successfully, but these errors were encountered: