-
Notifications
You must be signed in to change notification settings - Fork 424
Updated Route and Net UI Elements #3236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…g logic is consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some minor coding comments. Otherwise looks good.
return false; //No hit | ||
|
||
//Print info about all nodes to terminal including neighboring non-configurable nodes | ||
VTR_LOG("%s\n", describe_rr_node(device_ctx.rr_graph, device_ctx.grid, device_ctx.rr_indexed_data, node, draw_state->is_flat).c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just double checking, does this produce a lot of log messages? Or does it produce a reasonable amount?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It prints out a reasonable amount as long as there are not too many neighboring non-configurable RR nodes I believe.
Thanks for the code review Alex! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good; I just have a few things I suggest documenting and/or checking.
As we discussed in the meeting, things to check / update:
- Should be able to select flylines or routing after the routing is complete; only flylines before that.
- Not sure if we need a hit and a highlighted value for each rr_node; should explain why both are needed in the comments more, or try to get rid of the 'hit' one to save memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, the changes look good. This can be merged once CI completes.
It seems like (in another issue) we could do the same vec-> change on the .highlighted data member. What do you think?
There are usually a lot more highlighted nodes than hit nodes, and I'm worried it might impact performance since searching through a set is more costly than looking it up in an array. Otherwise, I think this PR is ready to merge. I've fixed the net/rr_graph highlighting issues as well. The rr_graph was drawing over the nets, so I've swapped the order, and now the nets are on top. |
OK, let's merge as soon as CI passes. Other changes from the to-do list can go in new PRs. |
Description
I’ve updated the RRGraph UI elements to give users greater flexibility in controlling what’s displayed. Previously, the route UI used dropdown menus that only allowed selecting specific combinations of RR nodes/edges. These have now been replaced with checkboxes and a master toggle, allowing more granular control. I also added options to enable or disable highlighting for both nets and routing, along with checkboxes to show or hide intracluster and intercluster nets.
Additionally, I cleaned up parts of the code related to highlighting, net transparency, and callback functions. Several element names have been renamed. Please review them to ensure they make sense.
Testing
There are some issues with the highlighting when turning both show nets and show RR on, and I think it will be a bit difficult to fix. Should I make these two options mutually exclusive? I think it might be useful though in some cases to show both the RRGraph and routing at once.
Messed up highlighting when both highlight rr edges, display rr, and display nets are turned on:
