gui: fix bbox descriptor#10575
Conversation
Signed-off-by: LucasYuki <lucasyuki@yahoo.com.br>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: LucasYuki <46359888+LucasYuki@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request refactors several bounding box retrieval methods in dbDescriptors.cpp to return whether the bounding box is valid using !bbox.isInverted() instead of unconditionally returning true or tracking a boolean flag. It also adds a check in dbITerm.cpp to handle empty master pins. The reviewer feedback highlights two critical issues where applying coordinate transforms to inverted bounding boxes can cause integer overflow/underflow, corrupting the coordinates and bypassing the inversion checks. The reviewer suggests checking for inverted bounding boxes before applying transforms in both dbITerm::getBBox and DbBoxDescriptor::getBBox.
| if (term->getMPins().empty()) { | ||
| Rect bbox; | ||
| bbox.mergeInit(); | ||
| return bbox; | ||
| } | ||
| Rect bbox = term->getBBox(); |
There was a problem hiding this comment.
Instead of checking if getMPins().empty() and manually constructing an inverted bounding box, it is more robust and direct to check if the retrieved term->getBBox() is inverted. This handles both the empty pins case and any other scenario where the master terminal's bounding box is invalid, while also preventing the instance transform from being applied to an inverted bounding box (which would corrupt its coordinates due to overflow/underflow during translation/rotation).
Rect bbox = term->getBBox();
if (bbox.isInverted()) {
return bbox;
}| bbox = getObject(object)->getBox(); | ||
| const auto xform = getTransform(object); | ||
| xform.apply(bbox); | ||
| return true; | ||
| return !bbox.isInverted(); |
There was a problem hiding this comment.
If bbox is inverted, applying the transform via xform.apply(bbox) will corrupt the coordinates (due to integer overflow/underflow during translation/rotation of INT_MAX/INT_MIN), which can make the box appear non-inverted and cause !bbox.isInverted() to incorrectly return true. It is safer to check if bbox is inverted and return false before applying the transform.
bbox = getObject(object)->getBox();
if (bbox.isInverted()) {
return false;
}
const auto xform = getTransform(object);
xform.apply(bbox);
return true;
Summary
This PR fixes 2 bugs in the gui:
Changes:
Type of Change
Impact
[How does this change the tool's behavior?]
Now objects that don't have a bounding box don't show an invalid bounding box.
Verification
./etc/Build.sh).Related Issues
[Link issues here]