feat(gui): Enhance 3D Viewer with filled faces, shading, and selectio… by openroad-ci · Pull Request #9532 · The-OpenROAD-Project/OpenROAD

openroad-ci · 2026-02-24T02:05:02Z

fix the issue #9412

Visual Improvements (3D Viewer): * Upgraded rendering from wireframes to solid polygons for real chiplet volume.

Added Z-depth sorting (Painter's Algorithm) for proper occlusion.

Implemented face-normal shading for 3D depth perception.

Changed the default color palette from Yellow to Teal.

Selection Synchronization: * Mapped 3D faces to database objects (odb::dbObject*).

Selections are now fully synchronized across the 3D Viewer, Inspector, DRC Viewer, and MainWindow (Layout Viewer).

Stability (Bugfix): * Fixed a std::out_of_range crash in renderThread.cpp.

Replaced unsafe map::at() calls with .find() to safely handle new or unregistered layout objects during asynchronous rendering.

…n sync - Fix map::at out of range exception in renderThread - Apply clang-format to chiplet3DWidget, mainWindow, and renderThread Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>

gemini-code-assist

Code Review

The pull request introduces significant visual enhancements to the 3D Viewer, including filled polygons, depth sorting (Painter's Algorithm), and flat shading. It also implements selection synchronization between the 3D Viewer and other GUI components. A stability fix for an asynchronous rendering crash in renderThread.cpp is included. While the visual improvements are valuable, there are several performance concerns in the new rendering logic, particularly regarding redundant vertex transformations and inefficient selection set processing within the paintEvent. Additionally, a regression was identified where connection region checks in the 3D Blox checker were disabled. Adherence to C++ coding standards (using C++ casts instead of C-style casts) and avoiding magic numbers should also be addressed.

gemini-code-assist · 2026-02-24T02:08:08Z

src/odb/src/3dblox/checker.cpp

 void Checker::checkConnectionRegions(dbMarkerCategory* top_cat,
                                     const UnfoldedModel& model)
 {
-  auto describe = [](const UnfoldedRegion* r, dbMarker* marker) {
-    marker->addSource(r->region_inst);
-    marker->addShape(Rect(r->cuboid.xMin(),
-                          r->cuboid.yMin(),
-                          r->cuboid.xMax(),
-                          r->cuboid.yMax()));
-    return fmt::format("{}/{} (faces {})",
-                       r->parent_chip->name,
-                       r->region_inst->getChipRegion()->getName(),
-                       sideToString(r->effective_side));
-  };
-  int count = 0;
-  dbMarkerCategory* cat = nullptr;
-  for (const auto& conn : model.getConnections()) {
-    if (!isValid(conn)) {
-      if (!cat) {
-        cat = dbMarkerCategory::createOrReplace(top_cat, "Connection regions");
-      }
-      auto* marker = dbMarker::create(cat);
-      marker->addSource(conn.connection);
-      std::string msg = fmt::format("Invalid connection {}: {} to {}",
-                                    conn.connection->getName(),
-                                    describe(conn.top_region, marker),
-                                    describe(conn.bottom_region, marker));
-      marker->setComment(msg);
-      logger_->warn(utl::ODB, 207, msg);
-      count++;
-    }
-  }
-  if (count > 0) {
-    logger_->warn(utl::ODB, 273, "Found {} invalid connections", count);
-  }
 }


The logic for checkConnectionRegions has been completely removed, leaving an empty function. This check is essential for validating 3D Blox connections (XY overlap, mating surfaces, thickness). Disabling this check is a significant regression in the checker's functionality.

gemini-code-assist · 2026-02-24T02:08:08Z

src/gui/src/chiplet3DWidget.cpp

+    for (int j = 0; j < 4; ++j) {
+      const uint32_t idx = indices_faces_[i + j];
+      if (idx < vertices_.size()) {
+        const QVector3D view_pos = modelView * vertices_[idx].position;


Transforming vertices inside the face loop is inefficient as vertices are shared between multiple faces. It is better to transform all vertices once into a temporary buffer before starting the face loop to avoid redundant matrix-vector multiplications.

gemini-code-assist · 2026-02-24T02:08:08Z

src/gui/src/chiplet3DWidget.cpp

+        if (obj) {
+          odb::dbBlock* chiplet_block = nullptr;
+          if (obj->getObjectType() == odb::dbChipObj) {
+            chiplet_block = ((odb::dbChip*) obj)->getBlock();


Avoid using C-style casts. Use static_cast for better type safety and clarity when the object type is already known.

chiplet_block = static_cast<odb::dbChip*>(obj)->getBlock();

gemini-code-assist · 2026-02-24T02:08:08Z

src/gui/src/chiplet3DWidget.cpp

+          }
+
+          // Check selection set
+          for (const auto& sel : selection_) {


Iterating through the entire selection_ set for every face of every chiplet in every frame is O(N_faces * N_selection) and can lead to performance issues if the selection set is large. Consider pre-calculating a set of selected odb::dbObject* pointers at the beginning of paintEvent to allow for O(1) lookups inside the loop.

gemini-code-assist · 2026-02-24T02:08:08Z

src/gui/src/chiplet3DWidget.cpp

+              auto* marker = std::any_cast<odb::dbMarker*>(any_obj);
+              // Check ownership
+              if (chiplet_block) {
+                auto* category = marker->getCategory();
+                if (category) {
+                  auto* parent = category->getParent();
+                  if (parent == chiplet_block) {
+                    is_highlighted = true;
+                    break;
+                  }
+                }
+              }
+              // Check sources
+              for (odb::dbObject* src : marker->getSources()) {
+                if (src == obj) {
+                  is_highlighted = true;
+                  break;
+                }
+                // Check name match for ChipInst vs Inst
+                if (obj->getObjectType() == odb::dbChipInstObj
+                    && src->getObjectType() == odb::dbInstObj) {
+                  const auto* chip_inst = (odb::dbChipInst*) obj;
+                  const auto* inst = (odb::dbInst*) src;
+                  if (chip_inst && inst
+                      && chip_inst->getName() == inst->getName()) {
+                    is_highlighted = true;
+                    break;
+                  }
+                }
+              }
+              if (is_highlighted) {
+                break;
+              }
+            }


The logic for determining if a dbMarker selection applies to a chiplet is complex and duplicated for both the selection set and the focus object. This logic should be encapsulated in a helper function to improve maintainability and reduce code duplication.

References

Encapsulate the logic for calculating master symmetry in a helper function to avoid code duplication.

gemini-code-assist · 2026-02-24T02:08:08Z

src/gui/src/chiplet3DWidget.cpp

+
+  // Transform to View Space
+  for (auto& p : current_poly) {
+    p = modelView * p;


The vertices are being transformed by modelView again here, even though they were already transformed for depth sorting in paintEvent. Passing view-space coordinates to drawFace3D would avoid redundant matrix-vector multiplications.

gemini-code-assist · 2026-02-24T02:08:08Z

src/gui/src/chiplet3DWidget.cpp

+    // 1. Fixed Light Direction (in View Space)
+    //    We assume light comes from top-right-front relative to camera.
+    //    Normalized vector: (1, 1, 1) -> (0.577, 0.577, 0.577)
+    const QVector3D light_dir = QVector3D(0.5f, 0.5f, 1.0f).normalized();


Avoid using hardcoded magic numbers for the light direction. Define it as a named constant.

References

Define tunable parameters as named constants instead of using hardcoded magic numbers.

gemini-code-assist · 2026-02-24T02:08:08Z

src/mpl/src/SACoreSoftMacro.h

+                          int end_col);
+  bool isSegmentEmpty(std::vector<std::vector<bool>>& grid,
+                      int start_row,
+                      int tart_col,


Typo in parameter name: tart_col should be start_col to match the implementation and maintain consistency.

Suggested change

int tart_col,

int start_col,

gemini-code-assist · 2026-02-24T02:08:08Z

src/mpl/src/SACoreSoftMacro.cpp

-            if (expanded_vicinity.total() > current_vicinity.total()
-                || expanded_vicinity == current_vicinity) {
-              current_vicinity = expanded_vicinity;
+              && isSegmentEmpty(grid, start_row, start_col, end_row, end_col)) {


Checking the entire segment from start_row to end_row is redundant because the previous rows were already verified to be empty in previous iterations of the while loop. Only the new row end_row needs to be checked for efficiency.

Suggested change

&& isSegmentEmpty(grid, start_row, start_col, end_row, end_col)) {

&& isSegmentEmpty(grid, end_row, start_col, end_row, end_col)) {

References

When analyzing code within a loop, consider the entire loop structure. Operations at the beginning of the loop (e.g., journalBegin) can affect the logic within the loop body, potentially making similar calls inside the loop redundant.

gemini-code-assist · 2026-02-24T02:08:08Z

src/pdn/src/grid.cpp

+  auto obs_filter = [this](const ShapePtr& other) -> bool {
+    if (other->shapeType() != Shape::GRID_OBS) {
+      return true;
+    }
+    const GridObsShape* shape = static_cast<GridObsShape*>(other.get());
+    return !shape->belongsTo(this);
+  };


Helper logic should be defined as a free function in a namespace rather than as a local lambda within a function to improve reusability and adhere to the project's coding standards.

References

Helper logic should be defined as a free function in a namespace rather than as a local lambda within a function.

github-actions · 2026-02-24T02:16:10Z

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

QuantamHD · 2026-02-24T06:30:06Z

src/gui/src/chiplet3DWidget.cpp

+  // Sort back-to-front (smaller Z in view space is further away if looking down
+  // -Z, but standard OpenGL view is looking down -Z. View space Z usually is
+  // negative in front of camera. So "further away" means more negative. Wait,
+  // let's check coordinate system.
+  // Standard: Camera at origin looking at -Z.
+  // Objects at Z=-10 are further than Z=-5.
+  // We want to draw -10 first. So sort ascending Z (more negative first).
+  std::ranges::stable_sort(
+      sorted_faces,
+      [](const FaceDepth& a, const FaceDepth& b) { return a.depth < b.depth; });


This is not a great way to implement depth based rendering. It's not always possible to sort triangles by depth. Example:

Instead you want to build a Z-buffer, which you store the depth of the closest pixel you've seen so far. If you see another triangle with a depth deeper than the current value in the zbuffer you continue.

https://haqr.eu/tinyrenderer/z-buffer/

QuantamHD · 2026-02-24T06:38:38Z

It looks like some of this code might have come from an LLM, overall it looks pretty good, but I would suggest restructuring the rendering code into a vertex and fragment shader. Right now you're mixing the two in the drawFaces3D function.

In most rendering pipelines you want a separation of concerns.

QuantamHD · 2026-02-24T07:19:49Z

Another reason to use the Z-Buffer is to ensure that the grid is correctly rendered with depth. Right now it looks like the grid is below both chips, but the reality is the grid is between the two chips.

feat(gui): Enhance 3D Viewer with filled faces, shading, and selectio…

f42c8d8

…n sync - Fix map::at out of range exception in renderThread - Apply clang-format to chiplet3DWidget, mainWindow, and renderThread Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>

openroad-ci assigned jferreiraOpenRoad Feb 24, 2026

gemini-code-assist bot reviewed Feb 24, 2026

View reviewed changes

QuantamHD reviewed Feb 24, 2026

View reviewed changes

	&& isSegmentEmpty(grid, start_row, start_col, end_row, end_col)) {
	&& isSegmentEmpty(grid, end_row, start_col, end_row, end_col)) {

Comments

Conversation

openroad-ci commented Feb 24, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 24, 2026

Uh oh!

QuantamHD Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

QuantamHD commented Feb 24, 2026

Uh oh!

QuantamHD commented Feb 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants