Skip to content
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

Inspector: add width, height for convenience alongside BBox #5025

Merged

Conversation

oharboe
Copy link
Collaborator

@oharboe oharboe commented May 1, 2024

Example use case: compare dimensions of two macros.

image

Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Copy link
Contributor

github-actions bot commented May 1, 2024

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

Comment on lines 1445 to 1446
props.push_back({"Width", std::string(Descriptor::Property::convert_dbu(bbox.dx(), false))});
props.push_back({"Height", std::string(Descriptor::Property::convert_dbu(bbox.dy(), false))});
Copy link
Collaborator

Choose a reason for hiding this comment

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

You might want to consider making these "BBox height and width" since are already used in several of the descriptors and overlapping names makes it impossible to access these from the tcl.

Copy link
Member

Choose a reason for hiding this comment

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

Are the properties not scoped by the type they refer to?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am out of my depth here... I just hacked things until I saw something convenient in the GUI....

It is probably easier for someone skilled.in the art to make the two line change than to train me to do it.

Should I close this PR?

Copy link
Member

Choose a reason for hiding this comment

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

I think he is just referring to the string "Width" versus "BBox Width" or the like. We can probably work it out in this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

could I append width/height to BBox or separate line?

I am confused about how Tcl enters into this.

Copy link
Member

Choose a reason for hiding this comment

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

There are GUI commands in tcl that can access the properties. @gadfort implemented it so he is best to answer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no scoping, i was just suggesting renaming the text from "Width" to "BBox width" (same for height) to avoid property name collisions. Shouldnt require much effort.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OIC

Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Copy link
Contributor

github-actions bot commented May 2, 2024

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

@oharboe
Copy link
Collaborator Author

oharboe commented May 2, 2024

image

@oharboe oharboe requested review from gadfort and maliberty May 2, 2024 05:32
@maliberty
Copy link
Member

Missing )
image

@oharboe
Copy link
Collaborator Author

oharboe commented May 2, 2024

Fixed.

Copy link
Contributor

github-actions bot commented May 2, 2024

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

Copy link
Member

@maliberty maliberty left a comment

Choose a reason for hiding this comment

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

@gadfort ok?

@maliberty
Copy link
Member

Missing DCO

Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
@oharboe
Copy link
Collaborator Author

oharboe commented May 2, 2024

Missing DCO

Fixed, force pushed.

Copy link
Contributor

github-actions bot commented May 2, 2024

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

@maliberty maliberty merged commit 9a0d811 into The-OpenROAD-Project:master May 3, 2024
11 checks passed
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.

None yet

3 participants