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

feat: Add helper text button for all nodes #3662

Closed
wants to merge 0 commits into from

Conversation

robinjhuang
Copy link
Collaborator

Adding the ? button to every node if it exports a DESCRIPTION field in the node definition file. This value is already sent over in the object_info endpoint.

Screenshot 2024-06-05 at 3 05 42 PM

Clicking on the ? shows the helper text:

Screenshot 2024-06-05 at 3 08 09 PM

Behavior:

  • Only displays ? if the node exports a DESCRIPTION field.
  • Shows helper text onclick
  • Wraps text so that it fits the width of the node. Thank you @Amorano

Known issues:

  • The text that pops up is sometimes hidden by a nearby node

@robinjhuang robinjhuang changed the title Feature: Add helper text button for all nodes feat: Add helper text button for all nodes Jun 6, 2024
@shawnington
Copy link
Contributor

Im going to say no, only because it will not be a feature that will be available except for the select few nodes that implement it, and a help function that is unused is worse than no help function.

@Amorano
Copy link

Amorano commented Jun 6, 2024

Im going to say no, only because it will not be a feature that will be available except for the select few nodes that implement it, and a help function that is unused is worse than no help function.

Say no to something that already exists? The meta tag for the field data is already apart of the ComfyUI Node spec -- so if there is no "documentation" its because the developer was too lazy to even put a single sentence into the string. Can't fix that.

Can fix that the "?" should only SHOWUP for nodes that have the string -- that is an easy tweak to the code I see above, just moving down the code that makes the "?" for the node beneath the logic that is testing for the documentation string.

Otherwise, that is just a free "tooltip".

Personally I prefer the solution from melMass:

image

Which is how we ended up here I believe. If there is a downside to showing the existing info (also being used in the ComfyUI Node Browser:

image

I made it a point at the summit to press on about all sorts of UX that should be getting some sharpening passes.

Bubble up the concerns!

@shawnington
Copy link
Contributor

shawnington commented Jun 6, 2024

Im going to say no, only because it will not be a feature that will be available except for the select few nodes that implement it, and a help function that is unused is worse than no help function.

Say no to something that already exists? The meta tag for the field data is already apart of the ComfyUI Node spec -- so if there is no "documentation" its because the developer was too lazy to even put a single sentence into the string. Can't fix that.

Can fix that the "?" should only SHOWUP for nodes that have the string -- that is an easy tweak to the code I see above, just moving down the code that makes the "?" for the node beneath the logic that is testing for the documentation string.

Otherwise, that is just a free "tooltip".

Personally I prefer the solution from melMass:

image

Which is how we ended up here I believe. If there is a downside to showing the existing info (also being used in the ComfyUI Node Browser:

image

I made it a point at the summit to press on about all sorts of UX that should be getting some sharpening passes.

Bubble up the concerns!

Unfortunately that is a reason. The idea is great if implemented uniformly and forced on custom nodes. But it wont be, so there is reality. There are ideas, and then there is reality. The reality is that you have to get the entire network of custom nodes to conform to this for it to be useful because the amount of default nodes people use is very minimal.

That has to be considered in a repo like this, that most the content is 3rd party and relies on this repo being consistent and stable.

This falls inline with a custom extension, that kind of extension of the js can be done by nodes. If you implement a node, you can provide the custom js extensions to make your node have that behavior, changes are not required of the repo to enable it.

Which is the differentiation in my opinion of requiring new features. If you can't implement it 3rd party, and you need new functionality to implement it. But your changes are all extensible JS, you just didn't write them that way.

But you can definitely write a node that implement this, with no changes. Obviously not with the hints types you proposed be defined in the nodes.py or custom_node.py your your_node.py, but your node can have .js so it's doable, so there is no need to implement something you can already do.

I do however agree with you that a change similar to this need to be made to the core, the custom node situation is a mess, and not having knowledge of exactly what they do is a problem. I just don't think this is the implementation.

It needs a much broader discussion. There are to many node creators out there.

Id also support the hints being modal instead of expanding the mode, lots of complex workflows have space as a premium and a node that expands with help info, needs more concrete behavior.

@Amorano
Copy link

Amorano commented Jun 6, 2024

Im going to say no, only because it will not be a feature that will be available except for the select few nodes that implement it, and a help function that is unused is worse than no help function.

Say no to something that already exists? The meta tag for the field data is already apart of the ComfyUI Node spec -- so if there is no "documentation" its because the developer was too lazy to even put a single sentence into the string. Can't fix that.
Can fix that the "?" should only SHOWUP for nodes that have the string -- that is an easy tweak to the code I see above, just moving down the code that makes the "?" for the node beneath the logic that is testing for the documentation string.
Otherwise, that is just a free "tooltip".
Personally I prefer the solution from melMass:
image
Which is how we ended up here I believe. If there is a downside to showing the existing info (also being used in the ComfyUI Node Browser:
image
I made it a point at the summit to press on about all sorts of UX that should be getting some sharpening passes.
Bubble up the concerns!

Unfortunately that is a reason. The idea is great if implemented uniformly and forced on custom nodes. But it wont be, so there is reality. There are ideas, and then there is reality. The reality is that you have to get the entire network of custom nodes to conform to this for it to be useful because the amount of default nodes people use is very minimal.

That has to be considered in a repo like this, that most the content is 3rd party and relies on this repo being consistent and stable.

This falls inline with a custom extension, that kind of extension of the js can be done by nodes. If you implement a node, you can provide the custom js extensions to make your node have that behavior, changes are not required of the repo to enable it.

Which is the differentiation in my opinion of requiring new features. If you can't implement it 3rd party, and you need new functionality to implement it. But your changes are all extensible JS, you just didn't write them that way.

But you can definitely write a node that implement this, with no changes. Obviously not with the hits defined in the nodes.py or custom_node.py your your_node.py, but your node can have .js so it's doable, so there is no need to implement something you can already do.

I guess I am still confused.

The information already exists. Just like if a widget is hidden. You can literally decide, as the author of the node, to add the doc string and have the tooltip rollover popup for your users.

I still see no downside. If your concern is the implementation -- that there should be a callback? to draw the "help button" -- that makes sense.

Outright not liking the idea because your perception people wont employ it is not really a reason.

I am not sold on the implementation, but the execution of some type of tooltip/help system IS a core item in most modern DCC applications -- even browser apps.

@shawnington
Copy link
Contributor

shawnington commented Jun 6, 2024

Im going to say no, only because it will not be a feature that will be available except for the select few nodes that implement it, and a help function that is unused is worse than no help function.

Say no to something that already exists? The meta tag for the field data is already apart of the ComfyUI Node spec -- so if there is no "documentation" its because the developer was too lazy to even put a single sentence into the string. Can't fix that.
Can fix that the "?" should only SHOWUP for nodes that have the string -- that is an easy tweak to the code I see above, just moving down the code that makes the "?" for the node beneath the logic that is testing for the documentation string.
Otherwise, that is just a free "tooltip".
Personally I prefer the solution from melMass:
image
Which is how we ended up here I believe. If there is a downside to showing the existing info (also being used in the ComfyUI Node Browser:
image
I made it a point at the summit to press on about all sorts of UX that should be getting some sharpening passes.
Bubble up the concerns!

Unfortunately that is a reason. The idea is great if implemented uniformly and forced on custom nodes. But it wont be, so there is reality. There are ideas, and then there is reality. The reality is that you have to get the entire network of custom nodes to conform to this for it to be useful because the amount of default nodes people use is very minimal.
That has to be considered in a repo like this, that most the content is 3rd party and relies on this repo being consistent and stable.
This falls inline with a custom extension, that kind of extension of the js can be done by nodes. If you implement a node, you can provide the custom js extensions to make your node have that behavior, changes are not required of the repo to enable it.
Which is the differentiation in my opinion of requiring new features. If you can't implement it 3rd party, and you need new functionality to implement it. But your changes are all extensible JS, you just didn't write them that way.
But you can definitely write a node that implement this, with no changes. Obviously not with the hits defined in the nodes.py or custom_node.py your your_node.py, but your node can have .js so it's doable, so there is no need to implement something you can already do.

I guess I am still confused.

The information already exists. Just like if a widget is hidden. You can literally decide, as the author of the node, to add the doc string and have the tooltip rollover popup for your users.

I still see no downside. If your concern is the implementation -- that there should be a callback? to draw the "help button" -- that makes sense.

Outright not liking the idea because your perception people wont employ it is not really a reason.

I am not sold on the implementation, but the execution of some type of tooltip/help system IS a core item in most modern DCC applications -- even browser apps.

So there are two reason, help info should be standardized, otherwise its basically useless. So if there is not a standardized template, it's just going to lead to more issue requests here.

Also, yes, there should be a callback to draw the help button. I shouldn't show for things that have no help context. That is what I was hinting at.

I agree with you 100% we should be working towards standardizing things and proving a system like you propose, but also there is nothing in place. Implementations are all over the place, and the ecosystem is not really at a place where standardization of this kind is realistic.

There is already little to no documentation, most node creators basically you know... hack their way around existing functionality while not even adhering to pytorch best practices... so...

I agree with your work in principle, in reality, it's forward thinking for where we are, and if you can contribute this kind of change, maybe you can also contribute more incremental changes to move things in that direction.

Id love to see a place where this system is in the main branch. But like so many features, It probably wont go anywhere, for the aforementioned reason.

I hope I am wrong though.

@Amorano
Copy link

Amorano commented Jun 6, 2024

There is already little to no documentation, most node creators basically you know... hack their way around existing functionality while not even adhering to pytorch best practices... so...

I agree with your work in principle, in reality, it's forward thinking for where we are, and if you can contribute this kind of change, maybe you can also contribute more incremental changes to move things in that direction.

Id love to see a place where this system is in the main branch. But like so many features, It probably wont go anywhere, for the aforementioned reason.

I hope I am wrong though.

While I breathe, I hope.

But this is 100% why I havent personally pushed anything I have done as extensions, here. I feel the same; standards around the desired target would be a great place to start and kind of where they landed from everything I saw/heard/said myself.

I think everyone is in the right place to do the work and perhaps, as you mention, this is much more sandboxy than not. At this point I wish there was a way to just give out JS extensions easy like the Python side and I would just point people at the ones that exist from people like melMass and Kijai.

A lot of the UX is solved across a bunch of repos -- is a bummer there is no npm for js extensions inside a virtual python env 🥇 someday....

@shawnington
Copy link
Contributor

There is already little to no documentation, most node creators basically you know... hack their way around existing functionality while not even adhering to pytorch best practices... so...

I agree with your work in principle, in reality, it's forward thinking for where we are, and if you can contribute this kind of change, maybe you can also contribute more incremental changes to move things in that direction.

Id love to see a place where this system is in the main branch. But like so many features, It probably wont go anywhere, for the aforementioned reason.
I hope I am wrong though.

While I breathe, I hope.

But this is 100% why I havent personally pushed anything I have done as extensions, here. I feel the same; standards around the desired target would be a great place to start and kind of where they landed from everything I saw/heard/said myself.

I think everyone is in the right place to do the work and perhaps, as you mention, this is much more sandboxy than not. At this point I wish there was a way to just give out JS extensions easy like the Python side and I would just point people at the ones that exist from people like melMass and Kijai.

A lot of the UX is solved across a bunch of repos -- is a bummer there is no npm for js extensions inside a virtual python env 🥇 someday....

Agree 100% with everything you just said.

@storyicon
Copy link

storyicon commented Jun 6, 2024

I happened to notice this PR. I agree with promoting similar features in ComfyUI, as it would enhance the user experience in many ways. I would be delighted to see ComfyUI become an excellent open-source "product", rather than just a playground for various models and algorithms. Similar possibilities could include:

  • A more effective keyword matching algorithm in the node search bar.
  • Clearly displaying which plugin a given node originates from.
  • Equalizing the spacing between multiple nodes for better alignment.
  • Some excellent basic nodes in the community should be actively incorporated into the ComfyUI backbone to prevent repeated implementation over and over again.

Compared to conservatives, I am likely a reformist.

@rui40000
Copy link

rui40000 commented Jun 6, 2024

6238c9ea32b976b91d37987559197a7

Saw you guys talking about making help buttons and wondered if you guys have used and this plugin which is doing the same function:
https://github.com/CavinHuang/comfyui-nodes-docs

@CavinHuang
Copy link

I don't think this should be a forced type of implementation, which would frustrate a lot of cutting edge developers who are working on experimental features, it should be positioned as a feature used to enhance the creator experience. And it can be turned off and modified to suit your own notes.

@Amorano
Copy link

Amorano commented Jun 6, 2024

6238c9ea32b976b91d37987559197a7

Saw you guys talking about making help buttons and wondered if you guys have used and this plugin which is doing the same function: https://github.com/CavinHuang/comfyui-nodes-docs

A few of us are doing the same thing with the extension melMass built mine is actually using the internet to load the pages, so it has more functionality than just static help files buried in the repository.

What I would like is it be a single solution IN the core -- but it needs:

  1. to support more than a single solution (static vs dynamic pages)
  2. should have a callback to allow the string to rendered as what the developer wants e.g. string/md/html
  3. have callbacks for where the button goes (there is no callback to give things like a consistent potision -- its all raw JS draw calls)
  4. needs to have the entire canvas fixed since STRING boxes HIDE everything behind them:
    image

here I use a style trick on my panel to get around it, but the tooltip is buried without the styling -- this shouldnt be a thing since this is a canvas problem where the controls are being rendered last over everything.

  1. If this is the start of a pass to add functionality to the system to allow more dynamic/in-place help, then tooltips should also be a thing --
    image

literally just using a field I added to the params called "tooltip".

If we are going to do this, lets actually do this.

@Amorano
Copy link

Amorano commented Jun 6, 2024

6238c9ea32b976b91d37987559197a7

Saw you guys talking about making help buttons and wondered if you guys have used and this plugin which is doing the same function: https://github.com/CavinHuang/comfyui-nodes-docs

image

no jovimetrix nodes, feels bad.

If you need docs, cause your system wont parse my nodes? (Salt.ai's auto-documentation system has this problem as well), you can just go to the local route:

127.0.0.1:8188/jovimetrix/doc

And it has the scrape of all the nodes with any help (only mine -- maybe I should make one for all):

image

It is specifically made this way so I can update a local cache that I push to the cloud for everyone else, so I dont have 900 help .md's in the code repo. =D

@melMass
Copy link
Contributor

melMass commented Jun 6, 2024

subscribing

For me it makes sense to have a core handler for DESCRIPTION since it was exposed long ago (#1438).

If you want a bit more insights on the original implementation goals of the documentation widget you can check this PR's body:

I meant to make a similar PR for comfy but couldn't decide what to keep since not all features are needed (markdown parsing, __doc__ fallback etc).

This PR is a nice initiative, I would make it affect only the core nodes by default and provide a way for extension developers to hook to it or not.

@robinjhuang
Copy link
Collaborator Author

Thanks for the comments everyone! Not many custom nodes currently use the DESCRIPTION field since it's not being used in core yet. This PR will change that.

@melMass 's right that this PR will break existing custom nodes that currently add their own help text tooltips. We probably need a way to hide this for certain nodes or allow 3rd parties to opt-in.

@AngryLoki
Copy link

Small tooltips with some helpful information are useful.
Would be nice to have; but all solutions on screenshots above look too flashy... Font is bigger than node title, as beefy as on could imagine (font-weight: 900?), not just brighter than title, but sometimes even in a scorching pink color. Why? Just make muted-color 🛈 button, make sure it is properly aligned to the node title. https://visualblocks.withgoogle.com/ has a good example:
image
image

@Amorano
Copy link

Amorano commented Jun 7, 2024

Small tooltips with some helpful information are useful. Would be nice to have; but all solutions on screenshots above look too flashy... Font is bigger than node title, as beefy as on could imagine (font-weight: 900?), not just brighter than title, but sometimes even in a scorching pink color. Why? Just make muted-color 🛈 button, make sure it is properly aligned to the node title

Absolutely agreed the ? is obnoxious! Thanks for the derp moment.... much cleaner for now.

image

Cheers!

@melMass
Copy link
Contributor

melMass commented Jun 7, 2024

Small tooltips with some helpful information are useful.

I agree with that, the core one should be only about small text without images.
For mtb I needed markdown since I'm sourcing the help from my wiki directly:
image

help_too_big.mp4

On the same lines, I wouldn't make it 'stick' but only appear on hover/fade out on leave

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.

8 participants