Skip to content

Utilize measureText to determine title length.#159

Closed
AustinMroz wants to merge 1 commit intoComfy-Org:masterfrom
AustinMroz:master
Closed

Utilize measureText to determine title length.#159
AustinMroz wants to merge 1 commit intoComfy-Org:masterfrom
AustinMroz:master

Conversation

@AustinMroz
Copy link
Contributor

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
Contributor

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
Contributor 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
Contributor

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

@huchenlei
Copy link
Contributor

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)

@webfiltered
Copy link
Contributor

Replaced by #962

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