web: add rudy heatmap view as save_image -web display option - #11149
web: add rudy heatmap view as save_image -web display option#11149mikesinouye wants to merge 2 commits into
Conversation
Signed-off-by: Mike Inouye <mikeinouye@google.com>
![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 adds support for rendering the Estimated Congestion (RUDY) heatmap overlay in the web-based image saver, including updating the visibility options, layer composition order, and adding corresponding tests. The review feedback highlights two issues in the test setup: a potential duplicate dependency conflict in src/web/test/BUILD between //src/gui and //src/gui:gui_stub, and a dangling pointer risk in TestSaveImage.cpp where a globally registered lambda captures the transient test fixture instance.
| "//src/gui", | ||
| "//src/gui:gui_stub", |
There was a problem hiding this comment.
Both //src/gui and //src/gui:gui_stub are listed as dependencies for save_image_test. This can lead to duplicate symbol errors, ODR (One Definition Rule) violations, or redundant dependencies. If the test requires the real gui library (e.g., for heatmap registration), //src/gui:gui_stub should be removed. Otherwise, if the stub is sufficient, //src/gui should not be added.
There was a problem hiding this comment.
This seems wrong, other targets in this file have this behavior. I thought it was gui:gui_qt and :gui_stub that were mutually exclusive
Signed-off-by: Mike Inouye <mikeinouye@google.com>
|
@maliberty I don't think I noticed how the same image command was evolving. It feels like this command will be incredibly hard to manage since the options and the display controls are not connected to each other. I don't think approving this, but I suspect this will need to be reworked causing some breaking changes. |
|
@codex review |
|
@gadfort are you concerned about |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@maliberty yes, in the GUI it is directly linked to the display controls so they provide a one-to-one mapping, while here it looks like its been added as needed. I like the shorthand here, but I think it should be derived from the display controls still so we done loose the one to one mapping |
|
You added |
3 participants
Summary
Add rudy heatmap view as save_image -web display option. We have flows that run pre-route but we still benefit from early congestion hotspot information.
Type of Change
Impact
New -display_option rudy to save_image -web
Verification
./etc/Build.sh).