Skip to content

gpl: Factor out graphics use in gpl#8710

Merged
maliberty merged 1 commit intoThe-OpenROAD-Project:masterfrom
hongted:gpl-graphics-refactor
Oct 23, 2025
Merged

gpl: Factor out graphics use in gpl#8710
maliberty merged 1 commit intoThe-OpenROAD-Project:masterfrom
hongted:gpl-graphics-refactor

Conversation

@hongted
Copy link
Contributor

@hongted hongted commented Oct 22, 2025

No description provided.

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@hongted hongted marked this pull request as ready for review October 22, 2025 22:34
@hongted hongted force-pushed the gpl-graphics-refactor branch from 364ec3d to 33f9f4e Compare October 22, 2025 23:17
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Signed-off-by: Ted Hong <tedhong@google.com>
@hongted hongted force-pushed the gpl-graphics-refactor branch from 33f9f4e to 5c3fa7e Compare October 23, 2025 00:32
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@maliberty
Copy link
Member

Please don't force push after code review has started as it breaks the "changes since last review" feature in GH (the last reviewed commit is elimiated).

Comment on lines +33 to +34
// Create a new object of the same class.
virtual std::unique_ptr<AbstractGraphics> MakeNew(utl::Logger* logger) const
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we making copies of the graphics object rather than share one?

Copy link
Contributor Author

@hongted hongted Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's to match the existing behavior where each object has its own Graphics object. The graphics object does have internal state, but I've not done the analysis to see if every object can share one.

graphics_ = std::make_unique<Graphics>(log_,
this,
pbc,
nbc,
pbVec,
nbVec,
npVars_.debug_draw_bins,
npVars.debug_inst);

graphics_ = std::make_unique<Graphics>(log_);

graphics = std::make_unique<Graphics>(log_, pbc_, pbVec_);

static constexpr size_t kInvalidIndex = std::numeric_limits<size_t>::max();
size_t selected_ = kInvalidIndex;
bool draw_bins_ = false;
utl::Logger* logger_ = nullptr;
HeatMapType heatmap_type_ = Density;
LineSegs mbff_edges_;
std::vector<odb::dbInst*> mbff_cluster_;
Mode mode_;
gui::Chart* chart_{nullptr};

@hongted
Copy link
Contributor Author

hongted commented Oct 23, 2025

Please don't force push after code review has started as it breaks the "changes since last review" feature in GH (the last reviewed commit is elimiated).

Understood, I'll do so in the future.

Also, I don't know if it's the same as "changes since last review", you can see the updates after a force-push via the "compare" link in this discussion. It does include changes due to the rebase and not just the commit though.

@maliberty maliberty merged commit eebcc18 into The-OpenROAD-Project:master Oct 23, 2025
13 checks passed
@gudeh
Copy link
Contributor

gudeh commented Dec 8, 2025

I realized a small oversight on this change. We now only plot the GUI charts if in debug mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments