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

Utilize measureText to determine title length. #159

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

Conversation

AustinMroz
Copy link

While measureText is used frequently throughout LiteGraph, computeSize calls did not make use of it. As a result, the node size calculations are consistently incorrect for long titles, short titles, or titles that utilize characters of abnormal width such as emojis.

My assumption is that this was purely because a context with the required title font isn't available when the call is made, but it's easy to add one just for ensuring the accuracy of title text sizing.

While measureText is used frequently throughout LiteGraph, computeSize
calls did not make use of it. As a result, the node size calculations
are consistently incorrect for long titles, short titles, or titles that
utilize characters of abnormal width such as emojis.

My assumption is that this was purely because a context with the
required title font isn't available when the call is made, but it's easy
to add one just for ensuring the accuracy of text sizing.
@webfiltered
Copy link
Collaborator

Given LiteGraph's age, there's a good chance measureText was intentionally avoided for performance reasons, and non-uniformly patched in later. Probably no impact on modern hardware, though?

@AustinMroz
Copy link
Author

I did some quick profiling since it was an underlying conern of mine as well and am seeing a time cost of ~4μs per call. Even when performing an action that makes frequent calls to compute size (like resizing a node) the impact seems negligable (<.1% time utilization).

I'm not sure of an easy way to check if the memory utilization, though.

@webfiltered
Copy link
Collaborator

Even less than I was expecting. Memory would be negligible, if even measurable.

@huchenlei
Copy link
Member

Please fix the test failures. Thanks!

Some how the canvas.title_measurement_context.measureText(text).width evals to an object, which is wield.

 TypeError: Cannot convert object to primitive value

      3590 |                 if (canvas) {
      3591 |                     let width = canvas.title_measurement_context.measureText(text).width
    > 3592 |                     return width + LiteGraph.NODE_TITLE_HEIGHT
           |                            ^
      3593 |                 }
      3594 |                 return font_size * text.length * 0.6;
      3595 |             }

      at compute_text_size (../litegraph/src/litegraph.js:3592:28)
      at LGraphNode.compute_text_size [as computeSize] (../litegraph/src/litegraph.js:3538:38)
      at ComfyApp.computeSize (src/scripts/app.ts:2369:25)
      at tryCatch (src/scripts/app.ts:26:1062)
      at Generator.<anonymous> (src/scripts/app.ts:26:3008)
      at Generator.next (src/scripts/app.ts:26:1699)
      at asyncGeneratorStep (src/scripts/app.ts:27:70)
      at _next (src/scripts/app.ts:28:163)

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.

3 participants