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

"Large number of open WorkerModule requests, some may not be returning" warning #112

Closed
natarius opened this issue Mar 30, 2021 · 14 comments

Comments

@natarius
Copy link

Hi there, I am seeing a large amount of warnings coming from troika-worker.utils.esm.js with this message.

Screen Shot 2021-03-29 at 8 19 42 PM

Everything seems to be working fine though

any idea?

@lojjic
Copy link
Collaborator

lojjic commented Mar 30, 2021

Hmm, that warning doesn't trigger until there are >1000 open worker requests. Do you have a very large number of text instances?

@natarius
Copy link
Author

I do, but not over 1000 (at least not intentionally)

more like a couple hundred

Here is what I am rendering

Screen Shot 2021-03-30 at 11 03 39 AM

But so what you are saying that maybe there is a bug on my side where I create more instances than I actually need and once I cross the 1000 count it starts throwing an error?

@natarius
Copy link
Author

I have been setting instances that have no content to .visible = false

but I guess that still means the instance exists

would you recommend instead of using the .visible prop to remove unused instances entirely?

@lojjic
Copy link
Collaborator

lojjic commented Mar 30, 2021

Ahh, that could explain it. Any calls to .sync() will still be issued to the worker in the background even if visible:false, and they'll pile up.

I do think I'd personally opt to create/dispose the labels on demand, or at least prevent .sync() calls for hidden ones. Those worker requests all use the same worker so if it's busy processing hidden labels then that could delay the processing of your visible ones.

@lojjic
Copy link
Collaborator

lojjic commented Mar 30, 2021

I'm making some assumptions, though, not having seen your exact LabelView code. It's still possible that there's a bug on my end, so LMK what you find.

@natarius
Copy link
Author

Just tried to only call .sync() when .visible === true

but still getting the warnings

if instead I only create instances for for visible objects and destroy unused ones then the warnings go away

@lojjic
Copy link
Collaborator

lojjic commented Mar 30, 2021

Hmm, Text itself only calls sync() in onBeforeRender, which shouldn't trigger if visible=false.

Is your LabelView wrapper code available anywhere I can take a peek?

@natarius
Copy link
Author

natarius commented Mar 30, 2021

It's part of a closed source project

but here is the relevant class, which might be enough

import {Mesh, MeshBasicMaterial} from "three";
import R from "../../../../../../../../../../resources/Namespace";
import BaseView, {ViewType} from "./BaseView";
import {Text} from "troika-three-text";
import {GeometryInstances} from "../instances/GeometryInstances";
import {MaterialInstances} from "../instances/MaterialInstances";

export enum TextAlign {
    left = "left",
    right = "right",
    center = "center",
    justify = "justify",
}

export enum HorizontalAlign {
    left = "left",
    right = "right",
    center = "center",
}

export enum VerticalAlign {
    top = "top",
    topBaseline = "top-baseline",
    middle = "middle",
    bottomBaseline = "bottom-baseline",
    bottom = "bottom",
}

export interface ILabelData {
    content: string;
    fontFace?: string;
    fontSize?: number;
    alignment?: TextAlign;
    horizontalAlign?: HorizontalAlign;
    verticalAlign?: VerticalAlign;
    color?: string;
    opacity?: number;
    backgroundColor?: string;
    highlightable: boolean;
}

export default class LabelView extends BaseView {
    get viewObject() {
        return this._viewObject as any;
    }

    get content() {
        return this.textMesh.text;
    }

    set content(content: string) {
        if (content !== this.textMesh.text) {
            this.textMesh.text = content;
            this.textMesh.visible = !!content;

            this.syncIfVisible();
        }
    }

    private syncIfVisible() {
        // Better for perf: https://github.com/protectwise/troika/issues/112
        if (this.textMesh?.text && this.textMesh?.visible && this._isVisible) {
            this.textMesh.sync();
        }
    }

    set textAlign(alignment: TextAlign) {
        this.textMesh.textAlign = alignment;
        this.syncIfVisible();
    }

    set horizontalAlign(alignment: HorizontalAlign) {
        this.textMesh.anchorX = alignment;
        this.syncIfVisible();
    }

    set verticalAlign(alignment: VerticalAlign) {
        this.textMesh.anchorY = alignment;
        this.syncIfVisible();
    }

    private backgroundMesh: Mesh | undefined;
    private readonly textMesh: any;
    private readonly foregroundColor: string;
    private highlightable: boolean;

    constructor(labelData: ILabelData) {
        super(ViewType.Label);

        this.foregroundColor = labelData.color || R.colors.elements.labels.foreground;

        this.highlightable = labelData.highlightable;

        if (labelData.backgroundColor) {
            this.textMesh = this.createTextMesh(
                labelData.content,
                labelData.fontFace,
                labelData.fontSize,
                labelData.alignment,
                labelData.horizontalAlign,
                labelData.verticalAlign,
                this.foregroundColor,
                labelData.opacity,
                1);

            this.textMesh.addEventListener("synccomplete", () => {
                if (this.backgroundMesh) {
                    this._viewObject.remove(this.backgroundMesh);
                }

                if (this.textMesh.text) {
                    const bounds = this.textMesh.textRenderInfo.blockBounds;
                    this.backgroundMesh =
                        this.createBackgroundBox(bounds[0], bounds[1], bounds[2], bounds[3], 0);
                    this._viewObject.add(this.backgroundMesh);
                }
            });

        } else {
            this.textMesh = this.createTextMesh(
                labelData.content,
                labelData.fontFace,
                labelData.fontSize,
                labelData.alignment,
                labelData.horizontalAlign,
                labelData.verticalAlign,
                this.foregroundColor,
                labelData.opacity,
                0);
        }

        this.syncIfVisible();

        this._viewObject.add(this.textMesh);
    }

    public onFrameRendered(elapsedTime: number) {
        if (this.highlightable) {
            this.setLabelHighlightState();
        }
    }

    private setLabelHighlightState() {
        const labelMaterial = this.textMesh.material as MeshBasicMaterial;
        if (this._isHighlighted) {
            labelMaterial.color.set(R.colors.elements.labels.highlighted);
        } else {
            labelMaterial.color.set(this.foregroundColor);
        }
    }

    private createTextMesh(content: string,
                           fontFace = R.layout.labels.font_face.regular,
                           fontSize = R.layout.labels.font_size,
                           textAlign = TextAlign.center,
                           horizontalAlign = HorizontalAlign.center,
                           verticalAlign = VerticalAlign.middle,
                           color: string,
                           opacity = 1,
                           zLevel: number) {
        const labelMesh = new Text();

        labelMesh.font = fontFace;
        labelMesh.text = content || "";
        labelMesh.fontSize = fontSize;
        labelMesh.position.z = zLevel;
        labelMesh.color = color;
        labelMesh.textAlign = textAlign;
        labelMesh.anchorX = horizontalAlign;
        labelMesh.anchorY = verticalAlign;

        labelMesh.material.opacity = opacity;
        labelMesh.name = "LabelView";

        return labelMesh;
    }

    private createBackgroundBox(startX: number, startY: number, endX: number, endY: number, zLevel: number) {
        const paddingLeft = 1;
        const paddingRight = 2;
        const paddingTop = 1;
        const paddingBottom = 0;

        const width = Math.abs(startX) + Math.abs(endX) + paddingLeft + paddingRight;
        const height = Math.abs(startY) + Math.abs(endY) + (paddingTop + paddingBottom);

        const geometry = GeometryInstances.getInstance().getPlaneGeometryInstance(width, height);
        const material = MaterialInstances.getInstance().backgroundColorMaterial;
        const mesh = new Mesh(geometry, material);

        const posX = ((startX + endX) / 2) + (paddingLeft / 4);
        const posY = ((startY + endY) / 2) - (paddingBottom);

        // The z level of element in accordance with standards defined in resources
        mesh.position.set(posX, posY, zLevel);
        mesh.name = "LabelBackgroundBox";

        return mesh;
    }
}

@lojjic
Copy link
Collaborator

lojjic commented Mar 30, 2021

Hmm, my hunch would be syncIfVisible() is getting called before .visible is set to false, but it's hard to say for sure without knowing how your view system works.

Something else I'm noticing is that you're issuing syncIfVisible calls after every individual property change (content, textAlign, horizontalAlign, etc.) which will be issuing more calls to the worker than necessary. If your view system has a lifecycle where you can issue a single sync call after all individual properties are set, that would be much better.

@lojjic
Copy link
Collaborator

lojjic commented Apr 7, 2021

From what you've described, I think this warning is behaving as expected; it correctly highlighted a performance issue. I'm closing this issue; feel free to reopen it if you believe differently. Thanks!

@lojjic lojjic closed this as completed Apr 7, 2021
@natarius
Copy link
Author

Just getting back to this

So it turns out I easily have over 1000 text instances in my scene. But there just not all visible at the same time

Many of them just appear on mouse-over of certain objects

So I am wondering what the best way forward is here:

  1. create instances only for visible text and then unmount as soon as it becomes invisible?
  2. use ThreeJS CSS objects instead?

@lojjic
Copy link
Collaborator

lojjic commented Apr 26, 2021

Personally I'd go for the first option. I could be wrong but I don't think there's any benefit to having all those hidden label objects present in the scene graph.

@natarius
Copy link
Author

Got it

I need to be able to show them quickly on mouse hover of objects

Would you recommend just destroying them after use and creating new ones on hover or just remove them from the threeJs scene instead?

@lojjic
Copy link
Collaborator

lojjic commented Apr 30, 2021

Honestly I don't think there's going to be a noticeable difference either way. I'd start with whatever's simplest to manage and optimize only if you notice an issue.

Personally I've done the create/destroy approach a lot, and it's always plenty snappy for me.

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

No branches or pull requests

2 participants