Skip to content

Fix: edge visibility bug#13421

Merged
danielzhong merged 6 commits into
mainfrom
DanielZhong/EdgeVisibilityBug
Apr 21, 2026
Merged

Fix: edge visibility bug#13421
danielzhong merged 6 commits into
mainfrom
DanielZhong/EdgeVisibilityBug

Conversation

@danielzhong

@danielzhong danielzhong commented Apr 21, 2026

Copy link
Copy Markdown
Member

Description

Problem:

  1. GLB files produced by TilesetPublisher contain degenerate triangles — triangles where two or more vertices share the same position, giving them zero area. In buildTriangleAdjacency, the cross product of the two edge vectors for such a triangle is the zero vector. Calling Cartesian3.normalize on it throws:
    Error: normalized result is not a number

These have always existed in the data, but previously went unnoticed because face normal computation was done in the GPU shader, where normalize(vec3(0)) silently returns vec3(0).

#13110 moved face normal computation to the CPU, introducing the first JS call to Cartesian3.normalize on per-triangle cross products. Unlike the GPU, Cartesian3.normalize throws a DeveloperError when given the zero vector, causing the tile to fail to load entirely.

This fix makes Cesium defensively skip degenerate triangles, restoring the previous tolerance for imperfect mesh data regardless of its origin.

  1. Cleanup and simplify silhouette normal decoding

The previous implementation decoded GLB signed-byte normals, then immediately re-encoded them to 16-bit oct format, packed two into a Uint32, and later unpacked and decoded them back to floats before writing into the Float32Array vertex buffer. This round-trip (signed byte → oct16 → Uint32 → oct16 → float) introduced quantization loss and unnecessary complexity.

The normals are now decoded once — signed byte → normalized Cartesian3 — and stored directly in a plain JS array. Each edge lookup reads directly from this array. The final vertex buffer (Float32Array) is unchanged. The unused Cartesian2 and AttributeCompression imports are removed.

Issue number and link

https://github.com/CesiumGS/tilers/issues/2156

Testing plan

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

AI acknowledgment

  • I used AI to generate content in this PR
  • If yes, I have reviewed the AI-generated content before submitting

If yes, I used the following Tools(s) and/or Service(s):
GitHub Copilot

If yes, I used the following Model(s):
Claude 4.6 Sonnect

@danielzhong danielzhong self-assigned this Apr 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Thank you for the pull request, @danielzhong!

✅ We can confirm we have a CLA on file for you.


// Skip degenerate triangles (zero-area): their cross product is the zero
// vector, which cannot be normalized and would produce NaN face normals.
const crossMagnitudeSquared = Cartesian3.magnitudeSquared(scratchCross);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how often this is called, and how performance-critical it is. So the following might not be important, but I'll mention it anyhow: Performance purists could claim that the (squared) length is computed twice here now - once for the check, and once for the normalize call. And these purists could suggest to change the normalize call to

multiplyByScalar(... 1.0 / Math.sqrt(crossMagnitudeSquared) ...);

But my gut feeling is that this will not immediately affect actual performance in any way.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point, modified!

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens EdgeVisibilityPipelineStage against malformed mesh data and simplifies the silhouette-normal decoding path used by EXT_mesh_primitive_edge_visibility, preventing tiles from failing to load when degenerate triangles are present.

Changes:

  • Skip zero-area (degenerate) triangles during triangle adjacency/face-normal computation to avoid Cartesian3.normalize throwing.
  • Replace the previous silhouette-normal decode/re-encode/pack/unpack pipeline with a single decode-to-float representation.
  • Remove unused imports related to the removed encoding/packing path.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/engine/Source/Scene/Model/EdgeVisibilityPipelineStage.js
Comment thread packages/engine/Source/Scene/Model/EdgeVisibilityPipelineStage.js Outdated
Comment thread packages/engine/Source/Scene/Model/EdgeVisibilityPipelineStage.js Outdated

@markschlosseratbentley markschlosseratbentley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - make sure you mark the checklist items in the PR desc.

@danielzhong danielzhong added this pull request to the merge queue Apr 21, 2026
Merged via the queue into main with commit 6599bff Apr 21, 2026
9 checks passed
@danielzhong danielzhong deleted the DanielZhong/EdgeVisibilityBug branch April 21, 2026 19:37
kurtyoung-dev added a commit to kurtyoung-dev/cesium-webgpu that referenced this pull request Jun 11, 2026
…evert

Owner-directed re-framing of the ultra-review's Axis-B (fork-drift) findings.
Decision lens: fix forward + improve the product; future merge cost is NOT a goal.

New doc migration_doc/FORK_DRIFT_ANALYSIS_2026-06-11.md:
- DECLINES the review's mass-revert recommendation (NEW-FORK-MODERNIZATION-REVERT,
  NEW-CAMERA-UPDATEVIEWMATRIX-REVERT) — modernization is product direction, stays.
- Identifies upstream FIXES/FEATURES (v1.142) worth pulling in on their own merit:
  pickModel matrix + octDecode fixes (CesiumGS#13433), BufferPointCollection update fix
  (CesiumGS#13465), empty-imageryLayers guard, ground-prim showsUpdated (CesiumGS#13366), degenerate-
  triangle edge guard (CesiumGS#13421), panorama lighting (CesiumGS#13369), EXT_structural_metadata
  vector tiles (CesiumGS#13426), OffscreenCanvas imagery (CesiumGS#13297), sg-scan JSDoc lint (CesiumGS#13377).
  Flags upstream breaking changes for the eventual sync (Node 22, BufferPrimitiveCollection
  readonly props).
- Fix-forward the regressions our modernization introduced (Resource.contains,
  TimeIntervalCollection.contains, Animation.js childNodes) — NEW-FORK-MODERNIZATION-REGRESSIONS.
- Restore dropped JSDoc/rationale comments (Camera/SSCC, ShadowMap) + adopt sg-scan.
- Context stays ours (GraphicsContext class is the better architecture); PickId is a
  rebase-onto-upstream candidate iff our delta is small.

DEFERRED_WORK.md: re-framed the Axis-B HIGH section, added NEW-FORK-MODERNIZATION-REGRESSIONS,
marked the revert items DECLINED.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kurtyoung-dev added a commit to kurtyoung-dev/cesium-webgpu that referenced this pull request Jun 16, 2026
…iumGS#13421/CesiumGS#13369) + Camera/ShadowMap JSDoc restore

Tractable Phase-9 drift items per FORK_DRIFT_ANALYSIS_2026-06-11 (fix-forward
lens; the full v1.142 merge is OUT of scope). Each upstream pull was verified to
exist in our forked tree first and ported into our structure (not cherry-picked).

Upstream pulls:
- CesiumGS#13366 NEW-UPSTREAM-GROUNDPRIM-SHOWSUPDATED-13366: Batch.remove() now clears
  showsUpdated alongside subscriptions in both StaticGroundGeometryPerMaterialBatch
  and StaticGroundPolylinePerMaterialBatch. A show-change-then-remove before the
  next update left a stale entry → "Cannot read properties of undefined (id)".
- CesiumGS#13421 NEW-UPSTREAM-EDGE-DEGENERATE-13421: EdgeVisibilityPipelineStage face-normal
  loop now guards on cross-product magnitudeSquared (=== 0 || !isFinite) and skips
  the degenerate triangle instead of normalizing a zero vector — our
  Cartesian3.normalize throws DeveloperError on zero-area triangles, crashing tile
  load. Normalizes via multiplyByScalar(1/sqrt(magSq)) on the valid path.
- CesiumGS#13369 NEW-UPSTREAM-PANORAMA-LIGHTING-13369: EquirectangularPanorama appearance
  sets flat:true so scene lighting no longer darkens the equirectangular image.

JSDoc/comment restore (preserve-JSDoc rule; sourced verbatim from merge-base
0becdbf, re-indented onto our class form):
- NEW-CAMERA-JSDOC-RESTORE: restored 5 lost @example blocks + 37 public-method
  docblocks on Camera.js and maximumTiltAngle/isDestroyed/destroy on
  ScreenSpaceCameraController.js. @example 1→6.
- NEW-SHADOWMAP-COMMENT-RESTORE: ~25 stripped WHY-comments back on
  ShadowMapComputations.js (cascade split-mix, light-space sign convention,
  perspective-divide, resize layout diagrams, visibility/camera section headers).
  Comment-only.

sg-scan (CesiumGS#13377) deferred as NEW-SG-SCAN-ADOPT — adds @ast-grep/cli binary + 7
upstream-convention rules that would flag our intentionally-diverged files; not
low-risk for a drift stage.

Verify: build green, tsc clean. New specs: EdgeVisibility degenerate (17/17),
Panorama flat (8/8), GroundGeometry showsUpdated (9/9), GroundPolyline (10/10),
Camera (349/349), ShadowMap (50/50). upstream-regression-check.mjs extended to 26
checks (added ground-prim/edge/panorama source guards) — 26/26. Standing gates:
collections-regression PASS, sandcastle-smoke PASS (3), csm-cast-dispatch PASS;
csm-soft-shadow 5/6 (pre-existing NEW-CSM-CASCADE-GROUND-FIT parity ballpark,
unrelated to comment-only ShadowMap change).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.

4 participants