Fix: 3D Viewer improvements and formatting#10243
Fix: 3D Viewer improvements and formatting#10243openroad-ci wants to merge 4 commits intoThe-OpenROAD-Project:masterfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the 3D viewer from the Qt GUI to a web-based implementation using Three.js, introducing a new ThreeDViewerWidget and backend support for fetching chiplet data. It also integrates DRC highlighting into the 3D view and enhances the DRC widget with a file browser and automated refreshes via database observers. Feedback recommends bundling Three.js locally instead of using an external CDN to support offline environments and suggests replacing hardcoded magic numbers with named constants to improve maintainability.
| // SPDX-License-Identifier: BSD-3-Clause | ||
| // Copyright (c) 2026, The OpenROAD Authors | ||
|
|
||
| import * as THREE from 'https://esm.sh/three@0.160.0'; |
There was a problem hiding this comment.
Importing Three.js from an external CDN (https://esm.sh/three@0.160.0) makes the 3D viewer dependent on an internet connection. This can be problematic in restricted or air-gapped environments where OpenROAD is often deployed. Consider bundling the library or serving it locally from the server to ensure the tool remains functional offline.
| builder.field("width", w > 0 ? w : 100000); | ||
| builder.field("height", h > 0 ? h : 100000); | ||
| builder.field("thickness", thickness > 0 ? thickness : 10000); |
There was a problem hiding this comment.
Use named constants instead of hardcoded magic numbers for default chip dimensions. This improves maintainability and makes the intent of these fallback values clearer.
constexpr int kDefaultChipWidth = 100000;
constexpr int kDefaultChipHeight = 100000;
constexpr int kDefaultChipThickness = 10000;
builder.field("width", w > 0 ? w : kDefaultChipWidth);
builder.field("height", h > 0 ? h : kDefaultChipHeight);
builder.field("thickness", thickness > 0 ? thickness : kDefaultChipThickness);References
- Define tunable parameters as named constants instead of using hardcoded magic numbers.
| // min_box: cached tech pitch as "minimum visible size" threshold. | ||
| // Default to 200 DBU (0.2um at 1000 dbu/um) if no routing layer available. | ||
| if (min_box_ < 0) { | ||
| min_box_ = 200; |
There was a problem hiding this comment.
Replace the magic number 200 with a named constant for the default minimum box size to improve code readability.
constexpr int kDefaultMinBox = 200;
min_box_ = kDefaultMinBox;References
- Define tunable parameters as named constants instead of using hardcoded magic numbers.
|
clang-tidy review says "All clean, LGTM! 👍" |
openroad-ci
force-pushed
the
test-3D-Viewer-Violations
branch
from
915ff0c to
36966a1
Compare
|
Web 3D Viewer: Added a new 3D Viewer window for browser visualization. DRC Viewer Integration: Interactively integrated the new 3D Viewer with the DRC Viewer. Native 3D Viewer Removal: Removed the legacy 3D Viewer from the native GUI (src/gui). |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@osamahammad21 FYI |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
clang-tidy review says "All clean, LGTM! 👍" |
|
conflict to resolve |
openroad-ci
force-pushed
the
test-3D-Viewer-Violations
branch
from
a1ec8b3 to
bc81839
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
openroad-ci
force-pushed
the
test-3D-Viewer-Violations
branch
from
be68ec3 to
b93ac6a
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
jferreiraOpenRoad
requested review from
maliberty
and removed request for
maliberty
…ations Conflicts resolved: - src/web/src/main.js: merged Tcl-result shutdown handling with DRC-widget refresh-on-success, both running after a successful eval. - src/web/src/request_handler.cpp: ported the new 3D-viewer endpoint (handleGet3DData) and the per-shape DRC marker serialization from the hand-rolled JsonBuilder API to Boost.JSON, matching the master rewrite (PR The-OpenROAD-Project#10323). Removed the obsolete json_builder.h dependency. Behavior preserved from feature branch: - handleDRCMarkers continues to clear all markers' visibility on category select; the user opts in via individual or batch toggles. - Two upstream tests (DRCOverlayIncludesVisibleMarkers, UpdateCategoryVisibilityBatch) updated to assert the new contract. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
| src/charts-widget.js | ||
| src/timing-widget.js | ||
| src/drc-widget.js | ||
| src/3d-viewer-widget.js |
There was a problem hiding this comment.
A similar change will be needed in BUILD
Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
3 participants
fix the issue #9412