Addresses a portion of #10619, 2.5 Display controls — overlays, grids & misc#10878
Conversation
Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to save and restore the display-controls state of the web viewer via new Tcl commands (save_display_controls and restore_display_controls), C++ backend handlers, and frontend state serialization. It also adds a background color picker with a reset option, integrates "Focused nets guides" and "Highlight selected" toggles, and upgrades the scale bar to render as an inline SVG with ticks. The review feedback highlights a performance improvement opportunity: the background color input listener should use the input event only for real-time CSS previews and defer expensive operations (like cookie writes, 3D viewer re-renders, and server syncs) to the change event to prevent UI stuttering.
The color picker's 'input' event fires on every tick while dragging; writing the cookie, re-rendering the 3D viewer and syncing the server per tick causes UI stutter. Keep 'input' to the cheap CSS-var preview and persist once on 'change'. Addresses gemini-code-assist review feedback on The-OpenROAD-Project#10878. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
The test drives the WebServer lifecycle (initLogger), which pulls web_serve.cpp.o and its gui::Gui::* references into the link. Without LTO the CMake build fails with undefined references to gui::Gui::get / setHeadlessViewer / setChartFactory: those symbols live in the Qt gui library, which would drag the whole application into a unit test. Compile src/gui/src/stub.cpp directly into the test instead, mirroring the Bazel target's //src/gui:gui_stub dependency. Fixes the Docker/CMake CI build of The-OpenROAD-Project#10878. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces features to save and restore the web viewer's display-controls state to and from a JSON file, exposed via new Tcl commands and synced over WebSockets. It also adds a background color picker with a reset option, introduces toggles for focused net guides and selection highlights, and refactors the scale bar to render as an inline SVG with proper subdivisions. The review feedback suggests dynamically resolving the default background color from the active theme's CSS variables to support light theme users correctly, and defensively guarding against invalid dbuPerMicron values in the scale bar calculations.
| // is purely client-side (tiles are transparent). | ||
| // Mirrors the CSS default (--bg-map: #111) in the "#rrggbb" form an | ||
| // <input type="color"> requires. | ||
| const DEFAULT_BG_COLOR = '#111111'; |
There was a problem hiding this comment.
When the user is using the light theme, the default background color of the layout is light (e.g., #ffffff), but the color picker input is initialized to a hardcoded dark color (#111111). We can dynamically resolve the active theme's default background color by reading the computed style of --bg-map on document.documentElement and converting it to a hex string if it is in rgb(...) format.
| const DEFAULT_BG_COLOR = '#111111'; | |
| const computedBg = getComputedStyle(document.documentElement).getPropertyValue('--bg-map').trim(); | |
| const rgbMatch = computedBg.match(/^rgb\((\d+),\s*(\d+),\s*(\d+)\)$/); | |
| const DEFAULT_BG_COLOR = rgbMatch | |
| ? '#' + rgbMatch.slice(1).map(x => parseInt(x).toString(16).padStart(2, '0')).join('') | |
| : (isValidHexColor(computedBg) ? computedBg : '#111111'); |
| bgReset.title = 'Reset background to the theme default'; | ||
| bgReset.addEventListener('click', () => { | ||
| resetBackgroundColor(app); | ||
| bgInput.value = DEFAULT_BG_COLOR; |
There was a problem hiding this comment.
When resetting the background color, setting bgInput.value to a hardcoded DEFAULT_BG_COLOR will result in a mismatched color picker swatch if the user is in light theme (since the background resets to light, but the swatch becomes dark). We should dynamically resolve the background color of the active theme after removing the inline style override.
const computedBg = getComputedStyle(document.documentElement).getPropertyValue('--bg-map').trim();
const rgbMatch = computedBg.match(/^rgb\((\d+),\s*(\d+),\s*(\d+)\)$/);
bgInput.value = rgbMatch
? '#' + rgbMatch.slice(1).map(x => parseInt(x).toString(16).padStart(2, '0')).join('')
: (isValidHexColor(computedBg) ? computedBg : '#111111');| if (!Number.isFinite(pxPerDbu) || pxPerDbu <= 0 | ||
| || !Number.isFinite(targetPx) || targetPx <= 0) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
If dbuPerMicron is negative or zero, pxPerUm will be negative or zero, which causes niceRoundParts to receive invalid inputs and return NaN values. We should defensively guard against invalid dbuPerMicron values.
| if (!Number.isFinite(pxPerDbu) || pxPerDbu <= 0 | |
| || !Number.isFinite(targetPx) || targetPx <= 0) { | |
| return null; | |
| } | |
| if (!Number.isFinite(pxPerDbu) || pxPerDbu <= 0 | |
| || !Number.isFinite(targetPx) || targetPx <= 0 | |
| || (dbuPerMicron != null && (!Number.isFinite(dbuPerMicron) || dbuPerMicron <= 0))) { | |
| return null; | |
| } |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a33156cc3
ℹ️ 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".
| const scheduleSyncDisplayState = rafCoalesce(() => { | ||
| if (!app.websocketManager) return; | ||
| app.websocketManager.request({ | ||
| type: 'set_display_state', | ||
| state: serializeDisplayState(), |
There was a problem hiding this comment.
Push every saved control change to the server
When the viewer first loads, or when the user changes cookie-only controls such as layer visibility/selectability or 3D True Z, nothing calls this scheduler even though those cookies are included in the saved display state. In those scenarios save_display_controls reads the previous/empty viewer_hook_ cache, so it can warn that no state was received or write stale or_hidden_layers/or_use_true_z values; restoring then does not reproduce the current display. Please schedule an initial sync and invoke it from each control path that updates one of the saved cookies.
Useful? React with 👍 / 👎.
Review follow-ups (gemini-code-assist and Codex on The-OpenROAD-Project#10878): - Sync the saved display state from every path that writes one of its cookies: once at the end of the initial bootstrap (so save_display_controls works before any interaction), on layer visibility changes (the chiplet mirror only reached the sync when the visible-chiplet set changed), on layer selectability changes (which trigger no redraw at all), and on the 3D true-Z toggle. All go through the rAF-coalesced scheduler, which now also no-ops in static report mode. The background-color sync lives inside setBackgroundColor/resetBackgroundColor themselves, so callers cannot forget it. - Resolve the background-color picker's default from the active theme's computed --bg-map via getThemeDefaultBgColor() in theme.js (the owner of all --bg-map handling), using the new pure helper cssColorToHex that normalizes the #rgb / #rrggbb / rgb() forms a custom property can hold. Behavior is unchanged today (--bg-map is The-OpenROAD-Project#111 in both themes) but the swatch stays truthful if the themes ever diverge. - Guard computeScaleBar against a non-finite or non-positive dbu-per-micron, falling back to the same default used for a missing value instead of propagating NaN geometry. Validated live with headless Chrome driving the real viewer: with no interaction at all and after each toggle (layer visibility, selectability, background color), save_display_controls writes the current state. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>
Addresses a portion of #10619
2.5 Display controls — overlays, grids & misc
focused_nets_guidestoggle (Misc); the overlay draws guides forroute_guide_net_ids ∪ focus_net_ids, keeping the inspector's per-net "Show route guides" as a finer web-only controldrawSelectedgatehighlight_selectedtoggle (Misc, default on); gates the selection highlight on the server overlay and the client outline/pulse — hover stays independent, and the outline is preserved across off→ondrawScaleBarcomputeScaleBar()helper (unit-tested), smooth rAF updates during zoom--bg-map, persists in theor_bg_colorcookie, re-renders the 3D viewersave/restore_display_controlssave_display_controls/restore_display_controlscommands; clients continuously sync their state (set_display_state), restore broadcasts the saved JSON and reloads with the camera preserved