feat(web): timing cone, chart types, ruler styles, and text labels#10966
Conversation
Advance the Web GUI toward Qt parity for issue The-OpenROAD-Project#10619: - Timing cone (2.9): fanin/fanout cone on the layout with slack/depth coloring, flight lines, per-pin depth labels, and optional schematic sync. - Chart types (2.11): unified chart dropdown, net-length (HPWL) histogram, stacked slack-by-path-group, CSV/PNG export, and select-by-bin drilldown. - Rulers & labels (2.12): global ruler-style default (euclidian/manhattan), server-side text labels with Tcl add_label/delete_label/clear_labels and an interactive client label manager (create-by-click, drag, edit/delete), labels composited into overlay tiles and save_image. Adds C++ unit tests (TestTimingCone, TestCharts, TestLabels) and JS tests, registered in both CMake and Bazel. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
There was a problem hiding this comment.
Code Review
This pull request introduces several major features to the OpenROAD web interface, including server-side user text labels with interactive client-side editing, a timing cone overlay (fanin/fanout) with logic-depth and slack coloring, a net-length (HPWL) histogram, and stacked slack histograms for multiple path groups. The review feedback highlights critical issues in the STA timing graph traversal where input pins are skipped due to only querying pinDrvrVertex instead of falling back to pinLoadVertex. Additionally, potential server crashes from Boost.JSON's as_double() on integer values, label name collisions during auto-generation, and duplicate DOM elements in the inspector toolbar should be addressed.
| auto* pin_vertex = graph->pinDrvrVertex(pin); | ||
| if (pin_vertex == nullptr) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
In STA, input pins of instances only have a load vertex (pinLoadVertex), while output pins only have a driver vertex (pinDrvrVertex). By only querying graph->pinDrvrVertex(pin), walkConeLevels will get a nullptr for all input pins and skip them. This prematurely terminates the timing cone traversal at the first input pin, preventing it from walking deeper than one level. To fix this, fall back to pinLoadVertex if pinDrvrVertex is null.
auto* pin_vertex = graph->pinDrvrVertex(pin);
if (pin_vertex == nullptr) {
pin_vertex = graph->pinLoadVertex(pin);
}
if (pin_vertex == nullptr) {
continue;
}| sta::Vertex* vertex = graph->pinDrvrVertex(pin); | ||
| if (vertex != nullptr) { |
There was a problem hiding this comment.
Similarly, querying only pinDrvrVertex to retrieve the vertex for slack calculation means that input pins (which only have a pinLoadVertex) will return nullptr, and their slack values will not be retrieved. Fall back to pinLoadVertex if pinDrvrVertex is null to ensure slack is correctly annotated for input pins.
sta::Vertex* vertex = graph->pinDrvrVertex(pin);
if (vertex == nullptr) {
vertex = graph->pinLoadVertex(pin);
}
if (vertex != nullptr) {| const double lower = req.json.at("lower").as_double(); | ||
| const double upper = req.json.at("upper").as_double(); |
There was a problem hiding this comment.
In Boost.JSON, calling as_double() on a boost::json::value that was parsed as an integer (e.g., if the client sends 5 instead of 5.0) will throw a boost::system::system_error (specifically error::not_double). Since bin edges can be integers (especially when use_dbu is true), this will cause the request to fail with a server error. Consider safely extracting the double value by checking if the value is a double, an int64, or a uint64.
| const double lower = req.json.at("lower").as_double(); | |
| const double upper = req.json.at("upper").as_double(); | |
| auto get_double = [](const boost::json::value& v) { | |
| if (v.is_double()) { | |
| return v.get_double(); | |
| } | |
| if (v.is_int64()) { | |
| return static_cast<double>(v.get_int64()); | |
| } | |
| if (v.is_uint64()) { | |
| return static_cast<double>(v.get_uint64()); | |
| } | |
| throw std::runtime_error("value is not a number"); | |
| }; | |
| const double lower = get_double(req.json.at("lower")); | |
| const double upper = get_double(req.json.at("upper")); |
| std::string label_name = name; | ||
| if (label_name.empty()) { | ||
| label_name = "label" + std::to_string(next_label_id_++); | ||
| } | ||
| // Reject a duplicate name (mirrors the Qt GUI, which warns GUI-44). | ||
| for (const auto& l : labels_) { | ||
| if (l.name == label_name) { | ||
| return ""; | ||
| } | ||
| } |
There was a problem hiding this comment.
If a user manually creates a label with a name like 'label0', any subsequent auto-generated label that gets assigned that same ID will fail the duplicate check and return an empty string. To prevent this, the auto-generation logic should loop and increment next_label_id_ until a unique name is found.
std::string label_name = name;
if (label_name.empty()) {
bool duplicate = true;
while (duplicate) {
label_name = "label" + std::to_string(next_label_id_++);
duplicate = false;
for (const auto& l : labels_) {
if (l.name == label_name) {
duplicate = true;
break;
}
}
}
} else {
// Reject a duplicate name (mirrors the Qt GUI, which warns GUI-44).
for (const auto& l : labels_) {
if (l.name == label_name) {
return "";
}
}
}| requestAnimationFrame(() => { | ||
| const toolbar | ||
| = this._app.inspectorEl?.querySelector('.inspector-toolbar'); | ||
| if (!toolbar) return; | ||
| const btn = document.createElement('button'); | ||
| btn.className = 'inspector-btn'; | ||
| btn.title = 'Delete label'; | ||
| btn.innerHTML = | ||
| '<svg width="18" height="18" viewBox="0 0 24 24" fill="currentColor">' | ||
| + '<path d="M6 19c0 1.1.9 2 2 2h8c1.1 0 2-.9 2-2V7H6v12zM19 4h-3.5l-1-1h-5l-1 1H5v2h14V4z"/>' | ||
| + '</svg>'; | ||
| btn.addEventListener('click', () => this.deleteLabel(name)); | ||
| toolbar.appendChild(btn); | ||
| }); |
There was a problem hiding this comment.
When appending the 'Delete label' button to the inspector toolbar, there is no check to see if a delete button already exists. If _select is called multiple times or if the inspector toolbar is not completely cleared/recreated on update, multiple duplicate delete buttons can accumulate in the DOM. Consider checking if a delete button is already present before appending a new one.
requestAnimationFrame(() => {
const toolbar
= this._app.inspectorEl?.querySelector('.inspector-toolbar');
if (!toolbar) return;
if (toolbar.querySelector('.delete-label-btn')) return;
const btn = document.createElement('button');
btn.className = 'inspector-btn delete-label-btn';
btn.title = 'Delete label';
btn.innerHTML =
'<svg width="18" height="18" viewBox="0 0 24 24" fill="currentColor">'
+ '<path d="M6 19c0 1.1.9 2 2 2h8c1.1 0 2-.9 2-2V7H6v12zM19 4h-3.5l-1-1h-5l-1 1H5v2h14V4z"/>'
+ '</svg>';
btn.addEventListener('click', () => this.deleteLabel(name));
toolbar.appendChild(btn);
});…abels - timing cone: walk through input pins by falling back to pinLoadVertex when pinDrvrVertex is null (both in the level walk and in slack annotation), so the cone reaches beyond the first logic level and colors input pins by slack. - net-length histogram: read bin edges via a jsonToDouble helper that accepts integer JSON kinds, since JS serializes whole numbers without a decimal point and Boost.JSON's as_double() throws on int64/uint64. - labels: auto-generated names now skip ids already taken by a user-named label instead of returning an empty string on collision. - label manager: guard against stacking duplicate "Delete label" buttons in the inspector toolbar across re-selections. Add TestLabels.AutoNameSkipsUserTakenId. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
Addresses a portion of #10619
2.9 Timing cone on layout (fanin/fanout)
Gui::timingCone(Term, fanin, fanout)→TimingConeRendererover STAfindFaninPins/findFanoutPinsWebSocketRequest::kTimingCone+handleTimingCone;TimingReport::computeTimingConeseeds from the selected inst (backend also acceptspin_name), walks the STA graph withfindFaninPins/findFanoutPins+Vertex{In,Out}EdgeIterator, and returns nodes with per-pin logic depth and slack. A cone panel in the timing widget drives it (Fanin / Fanout, depth limits, color mode)TimingConeRenderercolors by slack viaSpectrumGenerator(Turbo)spectrumColor, ported tocolor.cppsolibwebneeds no Qt link); per-pin logic-depth labels are drawn toofanin_depth/fanout_depthinputs (0 = unlimited, Qt parity); the BFS stops at the requested levelopenroad-cone-syncevent2.11 Multiple chart types
gui::ChartsWidget(charts.cpp) with aQComboBoxmode selector, largely a single endpoint-slack histogramcharts-widget.jsunified dropdown listing built-in charts and engine-provided line charts fetched on demand (debug_charts); switching type re-renders in placeTimingReport::getSlackHistogramreturns one series per path group;_drawBarsrenders them stacked, with a unified path-group filter (_populatePathGroups). In-group endpoint counting fixed so stacked totals equal distinct endpointscomputeNetLengthHistogramovernetHpwlDbu(half-perimeter ofdbNet::getTermBBox), selectable from the same dropdownbuildChartCsv) and PNG (canvas.toDataURL) export for the active chartselect_fanout_bin, nets viahandleSelectNetLengthBin/select_net_length_bin2.12 Rulers & labels
Rulercarries a per-rulereuclidian_flag toggled while building (k); manhattan rulers draw an L-shaped segmentruler.jsvia theor_ruler_stylecookie + a Tools → Euclidian rulers toggle; manhattan rulers render the L polyline (_getManhattanJoinPt)rulersdisplay-control flag; kept and wired through the overlay requestgui::LabelviaGui::addLabel(x, y, text, …)/ Tcladd_label/delete_label, drawn byLayoutViewer::drawLabelsand included insave_imageTileGenerator(addLabel/updateLabel/deleteLabel/clearLabelsunder one mutex), Tcladd_label/delete_label/clear_labels, and WSadd_label/update_label/delete_label/clear_labels/list_labels. Labels composite into the overlay tiles and intosave_image(renderLabelTile), matching QtLabelManager: create-by-click, drag-to-reposition, and edit/delete via the Inspector; move/edit use a single atomicupdate_label(never delete+add), so a label can't be lost mid-edit