Skip to content

EdgeGenerator.getEdgesGenerator: the time-slice check is inverted (time - performance.now() > iterationTime) and time is never advanced — the outer mesh loop can never yield #787

Description

@stokuewe

Versions

@thatopen/components3.4.8 (verified against the published dist/index.mjs; source on main: packages/core/src/fragments/EdgeProjector/projection/EdgeGenerator.js — the vendored copy of three-edge-projection).

What happens

iterationTime has no effect on the per-mesh loop of EdgeGenerator.getEdgesGenerator. The guard is dead code, so the generator never yields between meshes; it only yields inside generateEdges while working on one mesh. On a scene with many meshes the edge-generation phase therefore runs in chunks whose length is set by the meshes, not by the configured budget — iterationTime is silently ignored exactly where it was meant to bound the work.

This is not a freeze (the inner generator still yields), but the knob is inert, and progress reporting / frame pacing during that phase is correspondingly coarser than requested.

Why

// packages/core/src/fragments/EdgeProjector/projection/EdgeGenerator.js — getEdgesGenerator()lettime=performance.now();for(leti=0;i<meshes.length;i++){if(time-performance.now()>iterationTime){yield;}

Two defects in three lines:

  • the subtraction is reversed — time - performance.now() is monotonically <= 0 while iterationTime is >= 0, so the branch is unreachable for any configuration;
  • time is captured once before the loop and never reassigned, so even with the comparison corrected the guard would fire on every iteration after the first budget expires, rather than once per slice.

Two independent confirmations that this is a typo and not intent:

1. The same file gets it right twice, in getIntersectionEdgesGenerator — including the time reassignment the broken copy lacks:

if(performance.now()-time>iterationTime){yield;time=performance.now();}

(and ProjectedEdgeCollector.addEdgesGenerator in ProjectionGenerator.js uses the same correct form).

2. Upstream is not affected. This directory is a vendored fork of gkjohnson/three-edge-projection; upstream's src/EdgeGenerator.js has the correct form in the corresponding loop:

lettime=performance.now();for(leti=0;i<meshes.length;i++){if(performance.now()-time>iterationTime){yield;time=performance.now();}

So the divergence was introduced in this copy — presumably alongside the ThatOpen-only changes in the same loop (the projectionDirection / localProjection handling, which upstream does not have). That is why this report is filed here rather than upstream: the shipped defect is only in @thatopen/components.

Reproduction sketch

constprojector=components.get(OBC.EdgeProjector);projector.generator.iterationTime=1;// ask for ~1 ms slicesconsole.time("edges");awaitprojector.get(modelIdMap,world,{onProgress: (msg,p)=>console.log(msg,p,performance.now()),});console.timeEnd("edges");

During the candidate-edge phase, the gaps between successive frames are governed by the per-mesh work, not by iterationTime; setting projector.generator.iterationTime to 10000 produces the same yielding behaviour for that loop as setting it to 1. Only the inner generateEdges slicing responds to the value. Instrumenting the loop (or stepping it in a debugger) shows the if body is never entered.

Suggested fix

Restore the upstream form:

if(performance.now()-time>iterationTime){yield;time=performance.now();}

Worth a quick grep across the vendored projection/ directory for the same inverted pattern while fixing it, since it is a single-character-class mistake that repeats easily.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions