Skip to content

EdgeProjector.cullerPixelsPerMeter: the TSDoc is inverted — the value is METERS PER PIXEL, so a higher value is coarser, not "more accurate" #788

Description

@stokuewe

Versions

@thatopen/components 3.4.8 (verified against the published dist/index.d.ts and dist/index.mjs; source on main: packages/core/src/fragments/EdgeProjector/index.ts, packages/core/src/fragments/EdgeProjector/projection/VisibilityCuller.js).

What happens

The documentation of EdgeProjector.cullerPixelsPerMeter says the opposite of what the code does, in both halves of the sentence. A user who raises the value "to increase accuracy", as the TSDoc instructs, makes each culling pixel larger, loses more small meshes from the projection, and makes the culling faster, not slower.

This is not cosmetic: the visibility culler drops any mesh without a single visible pixel from both the visible and the hidden line sets — so this field is the one knob governing how much small geometry a projection loses, and it is documented backwards.

Why

The doc comment:

// packages/core/src/fragments/EdgeProjector/index.ts
/**
 * Resolution of the visibility culler in pixels per meter.
 * Higher values = more accurate occlusion but slower culling.
 */
cullerPixelsPerMeter = 0.05;

The code it configures divides the world size by the value, which makes it the size of a pixel in metres:

// packages/core/src/fragments/EdgeProjector/projection/VisibilityCuller.js
const pixelWidth  = Math.ceil( size.x / pixelsPerMeter );
const pixelHeight = Math.ceil( size.z / pixelsPerMeter );
const tilesX = Math.ceil( pixelWidth / maxTextureSize );
const tilesY = Math.ceil( pixelHeight / maxTextureSize );
target.setSize( Math.ceil( pixelWidth / tilesX ), Math.ceil( pixelHeight / tilesY ) );

At the default 0.05 a culling pixel covers 5 cm. At 0.5 it covers 50 cm — ten times coarser per axis, ~100× fewer pixels, i.e. dramatically cheaper and dramatically less accurate. "Pixels per meter" would mean the reciprocal: 0.05 px/m would be one pixel per 20 metres, which is not what the code computes.

The vendored source agrees with the code, not with the wrapper's TSDoc. Upstream three-edge-projection's MeshVisibilityCuller documents the same field honestly:

/**
 * The size of a pixel on a single dimension. If this results in a texture larger than what
 * the graphics context can provide then the rendering is tiled.
 * @type {number}
 */
this.pixelsPerMeter = pixelsPerMeter;

So the misleading claim ("pixels per meter", "higher values = more accurate … but slower") is introduced by the EdgeProjector wrapper, on the public, typed surface users actually read.

Reproduction sketch

const projector = components.get(OBC.EdgeProjector);

projector.cullerPixelsPerMeter = 0.5;      // "more accurate", per the TSDoc
const coarse = await projector.get(modelIdMap, world);

projector.cullerPixelsPerMeter = 0.01;     // "less accurate", per the TSDoc
const fine = await projector.get(modelIdMap, world);

console.log(Object.keys(coarse.groups).length, coarse.visible.getAttribute("position").count);
console.log(Object.keys(fine.groups).length,   fine.visible.getAttribute("position").count);
// `fine` covers more source items and more segments, and takes longer:
// the "less accurate" setting is the accurate one.

Suggested fix

  1. Correct the TSDoc, e.g.:

    /**
     * Size of one visibility-culling pixel, in meters. LOWER values mean finer culling —
     * fewer small meshes are discarded — at the cost of a larger render target,
     * more tiles and more readback time. Default 0.05 (5 cm per pixel).
     *
     * Note: meshes with no visible pixel are removed from BOTH the visible and the
     * hidden line sets, so this value bounds how much small geometry a projection loses.
     */
  2. Preferably rename the public field to something that cannot be read backwards — cullerMetersPerPixel, same default 0.05. The current name is the origin of the misreading, and it is inherited from an upstream name that upstream itself has to explain away in prose. A rename on the wrapper does not require touching the vendored file (new EdgeProjectorVisibilityCuller(threeRenderer, { pixelsPerMeter: this.cullerMetersPerPixel })).

Related: #763 (EdgeProjector returning empty geometry for subsets) — the culler stage is also where that loss occurs, so documenting this knob correctly has more than editorial value.

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