GUI: Handle display controls for multiple loaded technologies (Web GUI)#10348
Conversation
|
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
Code Review
This pull request implements a hierarchical layer management system, enabling the web interface to display and control layers organized by chiplet instances. It includes backend serialization of the chip hierarchy and a recursive frontend implementation for the layer tree and visibility controls. Feedback highlights the need for stable layer IDs based on instance names, the removal of redundant visibility checks, and a more consistent ID construction scheme to simplify fragile matching logic in the inspector.
|
|
||
| if (hierarchyNode.instances && hierarchyNode.instances.length > 0) { | ||
| hierarchyNode.instances.forEach((inst, idx) => { | ||
| const instId = `${parentId}_inst_${idx}`; |
There was a problem hiding this comment.
Using an index-based ID (inst${idx}) makes the layer visibility state dependent on the order of instances in the JSON and breaks compatibility with the instance_path used by the inspector. It is better to use the instance name provided in the hierarchy node to ensure stable and predictable IDs.
| const instId = `${parentId}_inst_${idx}`; | |
| const instId = parentId + "/" + (inst.name || idx); |
| if (app.layerModel && layer_id) { | ||
| const nodeId = instance_path ? `layers_parent_${instance_path}_${layer_id}` : layer_id; | ||
| // Check layerModel visibility | ||
| if (app.layerModel.isVisible && !app.layerModel.isVisible(nodeId) && !app.layerModel.isVisible(`layers_parent/${instance_path}/${layer_id}`) && !app.layerModel.isVisible(`layers_parent_${instance_path}/${layer_id}`) && !app.layerModel.isVisible(`layers_parent_${instance_path}/${layer_id}`) && !app.layerModel.isVisible(instance_path ? `${instance_path}/${layer_id}` : layer_id)) { |
There was a problem hiding this comment.
This condition contains a duplicate check for layers_parent_${instance_path}/${layer_id}.
Additionally, the logic relies on multiple guesses for the layer ID format (using both _ and / as separators). This is fragile and suggests that the ID scheme is not unified across the application. It is recommended to use a consistent ID construction method (e.g., always using / as a separator) in both display-controls.js and inspector.js to avoid this complex and error-prone matching logic.
| const id = name; | ||
| layer._nodeId = id; | ||
|
|
||
| const visible = !savedHiddenLayers.has(name) && !savedHiddenLayers.has(id); |
b2bf1ca to
756c2e2
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
756c2e2 to
7e59f70
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
7e59f70 to
e982c5e
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
|
fix #10208 |
e982c5e to
31acd9d
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31acd9d46a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| node.data.layer.addTo(app.map); | ||
| app.visibleLayers.add(node.data.name); | ||
| app.visibleLayers.add(id); |
There was a problem hiding this comment.
Store backend layer names in visibleLayers
This callback now writes hierarchical node IDs (for example layers_parent/.../M1) into app.visibleLayers, but the same set is sent to the backend as visible_layers. The backend compares that filter against raw tech-layer names (layer->getName()), so after this change the filter values no longer match: net selection in selectAt drops all net hits, and pin rendering filters out all pin boxes whenever has_visible_layers is true. Keep UI node IDs separate from the wire-format layer-name filter (or translate IDs back to names before requests).
Useful? React with 👍 / 👎.
display-controls.js stored hierarchical UI node IDs (e.g. "layers_parent/<tech>/M1") in app.visibleLayers, but the backend filters compare against raw dbTechLayer::getName(). After PR The-OpenROAD-Project#10348 the filter values no longer matched, so net selection (selectAt), pin tile rendering and ruler snap all dropped every hit whenever a non-empty visible_layers was sent. Introduce app.visibleLayerNames as a parallel Set of raw layer names, kept in sync with the hierarchical IDs in the CheckboxTreeModel callback. The names set is rebuilt from scratch on every checkbox change so that two layers sharing a getName() across different techs don't drop each other. Route the WebSocket tile, select and snap requests through the new set; cookie persistence keeps using the hierarchical IDs. Adds a regression test in test-ruler.js asserting that the snap request payload carries raw names, not hierarchical paths. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8f7297842
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (trackChiplets && node.data && node.data.chipletPath) { | ||
| const path = node.data.chipletPath; | ||
| const want = !!node.checked; | ||
| if (app.visibleChiplets.has(path) !== want) { |
There was a problem hiding this comment.
Preserve chiplet visibility for indeterminate layer nodes
When a chiplet node in the Layers tree is partially selected, CheckboxTreeModel sets indeterminate=true and checked=false; this code treats that state as hidden (want = !!node.checked) and pushes it into chipletModel.checkSet. In multi-chiplet designs, unchecking a single layer under a chiplet can therefore remove the entire chiplet from visible_chiplets, so other still-checked layers from that chiplet disappear from tile/select rendering. The mirror logic should treat indeterminate chiplet nodes as visible (or compute visibility from descendant leaf layers) instead of coercing only checked.
Useful? React with 👍 / 👎.
Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
a8f7297 to
27fe6d1
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
fix #10209