Skip to content

Refactor node rendering, serialization, and graph model architecture#518

Open
imakris wants to merge 27 commits intopaceholder:masterfrom
imakris:claude/thorough-code-review-7uWFj
Open

Refactor node rendering, serialization, and graph model architecture#518
imakris wants to merge 27 commits intopaceholder:masterfrom
imakris:claude/thorough-code-review-7uWFj

Conversation

@imakris
Copy link

@imakris imakris commented Mar 25, 2026

Type of change

  • New feature
  • Breaking change
  • Documentation/refactoring

Description

This is a comprehensive refactoring that improves the architecture and rendering pipeline of the node editor:

Major Changes

Node Rendering System

  • Introduced NodeRenderingUtils with centralized node rendering constants and utilities
  • Implemented shadow atlas generation with multi-pass box blur for improved visual quality
  • Added DefaultNodeGeometryBase to consolidate shared geometry logic between horizontal and vertical layouts
  • Enhanced DefaultNodePainter with sophisticated shadow rendering and caching

Graph Model Architecture

  • Introduced ConnectionIdIndex for efficient connection lookups by node and port
  • Changed allNodeIds() return type from std::unordered_set<NodeId> to NodeIdSet const & for better performance (avoids copies)
  • Updated DataFlowGraphModel to use the new index-based connection tracking
  • Refactored serialization validation into dedicated SerializationValidation module with reusable utilities

Scene and Graphics

  • Split BasicGraphicsScene functionality: moved group-related methods to new BasicGraphicsSceneGroups.cpp
  • Enhanced GraphicsView with smooth zoom animation support and improved cache management
  • Improved ConnectionGraphicsObject with cached geometry rebuilding and better bounding rect calculations
  • Updated NodeGraphicsObject with rendering optimizations and graphics view integration

Serialization & Validation

  • Created SerializationValidation.hpp/cpp with robust JSON parsing utilities
  • Centralized node/group/connection ID parsing with proper error handling
  • Improved validation of serialized graph data with detailed error messages
  • Removed duplicate parsing logic across multiple files

Code Quality

  • Removed obsolete PySide2/Shiboken CMake modules
  • Cleaned up unused includes and forward declarations
  • Improved const-correctness and reference semantics
  • Added comprehensive namespace organization with anonymous namespaces for internal helpers

Breaking Changes

  1. AbstractGraphModel::allNodeIds() now returns NodeIdSet const & instead of std::unordered_set<NodeId>. This requires graph model implementations to maintain a member variable instead of computing the set on-the-fly.

  2. Connection tracking now uses ConnectionIdIndex internally. Custom graph models should leverage this for efficient connection queries.

  3. Serialization functions moved to SerializationValidation namespace. Code using jsonValueToNodeId() or similar functions should use the new detail::read_node_id() utilities.

Migration Guide

For custom graph models:

// Before
std::unordered_set<NodeId> allNodeIds() const override {
    std::unordered_set<NodeId> result;
    for (auto& p : _models) result.insert(p.first);
    return result;
}

// After
AbstractGraphModel::NodeIdSet const &allNodeIds() const override {
    return _nodeIds;  // Maintain as member variable
}

Testing

  • Qt version tested: 5.15+
  • Existing tests still pass
  • Added tests for zoom animation features
  • Serialization validation tests included

Related issue

Improves overall architecture and rendering performance of the node editor library.

https://claude.ai/code/session_01PgU81XZwKqXaPr628mVdQj

imakris and others added 27 commits March 24, 2026 09:49
- Add antialiasing to connection painting and grid background
- Use floating-point pen widths with round cap/join for connections
- Apply half-pixel offset to grid lines for crisp rendering
- Set cosmetic pen flag on grid lines so width stays constant across zoom
Replace the abrupt 1.2x discrete zoom steps with a velocity/friction
model: each wheel notch adds impulse, a 16ms timer decays velocity by
0.75x per frame while applying incremental scale changes around the
original wheel-event pivot point.
DeviceCoordinateCache on NodeGraphicsObject snaps rendering to
integer device pixels, causing visible jitter during smooth zoom.
Temporarily switch nodes to NoCache while the zoom timer is active
and restore DeviceCoordinateCache when it stops.
Scrollbar values are integers, so adjusting them during smooth zoom
causes the entire scene to jump by whole pixels each frame. Fix by
splitting the desired offset into integer scrollbar values and a
fractional translation baked into the view transform. The sub-pixel
translation is removed when the zoom animation stops.
Generate a blurred rounded-rect atlas once per (shadow color, DPR) and
draw it as 9 stretched tiles per node.  This gives smooth soft falloff
at the same cost as the previous 3-rect approximation (~0.85ms release)
and is ~19x cheaper than QGraphicsDropShadowEffect.

Remove the QGraphicsDropShadowEffect code path from NodeGraphicsObject.
Re-enable ShadowEnabled in DefaultStyle.json.  Expand bounding rect
margins in both geometry classes to accommodate the shadow extent.
- Fix bug in NodeGraphicsObject::hoverMoveEvent: `|` -> `&` for flag test
  (line 466 was always true due to bitwise OR with Resizable)
- Replace triplicated JSON style read/write macros across NodeStyle,
  ConnectionStyle, and GraphicsViewStyle with shared helpers in Style.hpp
- Extract portCountRole() helper to eliminate repeated ternary expressions
  across 7 files
- Extract DataFlowGraphModel::connectDelegateModel() to deduplicate ~30
  lines of identical signal connection setup in addNode/loadNode
- Remove unused hash specializations (pair/tuple) from ConnectionIdHash.hpp
- Remove commented-out code from NodeGraphicsObject, ConnectionGraphicsObject,
  ConnectionState, and Style.hpp
- Remove unused NODE_EDITOR_DEMANGLED macro from Export.hpp
- Remove unused PySide2/Shiboken2 cmake modules (never referenced)
- Simplify oppositePort() switch with direct returns

Net reduction: ~715 lines across 20 files.

https://claude.ai/code/session_015aZAxLSpakeTpF7zaq6D2F
- Remove phantom forward declarations for non-existent classes:
  NodeDataModel, ConnectionGeometry, GraphModel, NodeGeometry
- Remove redundant forward declarations where the header is already
  included (QPainter, ConnectionId, DataFlowGraphModel, etc.)
- Remove unused forward declarations (NodeStyle, NodeState,
  StyleCollection, NodeDelegateModel, NodeConnectionInteraction, etc.)
- Remove dead methods: connectionRequiredPort(), nodePortScenePosition(),
  connectionEndScenePosition() in NodeConnectionInteraction;
  addGraphicsEffect() in ConnectionGraphicsObject
- Remove #if 0 blocks in NodeDelegateModelRegistry.hpp (old registerModel
  overloads, type converter API)
- Remove unused includes: QUuid (3 headers), QUuidStdHash, iostream,
  QFile, QMessageBox, QMargins, QPointF, QIcon, QJsonObject,
  NodeConnectionInteraction, NodeState, ConnectionGraphicsObject
- Remove duplicate includes: NodeState.hpp (NodeGraphicsObject.hpp),
  QColor (NodeDelegateModel.hpp)
- Remove unused using declarations and variables: DataFlowGraphModel,
  NodeConnectionInteraction, NodeDataType, QVariant result in setPortData

https://claude.ai/code/session_015aZAxLSpakeTpF7zaq6D2F
- Fix include/QtNodes/NodeGeometry: was pointing to non-existent
  internal/NodeGeometry.hpp, now correctly points to
  internal/AbstractNodeGeometry.hpp
- Remove unused QVariant includes from QUuidStdHash.hpp and
  QStringStdHash.hpp
- Remove unused QObject include from Style.hpp

https://claude.ai/code/session_015aZAxLSpakeTpF7zaq6D2F
1. Extract DefaultNodeGeometryBase from duplicated geometry classes

   DefaultHorizontalNodeGeometry and DefaultVerticalNodeGeometry shared
   ~120 lines of identical code (constructor, boundingRect, size,
   captionRect, portTextRect, maxPortsTextAdvance, maxPortsExtent, and
   all 4 member variables). Extract these into a new intermediate base
   class DefaultNodeGeometryBase, leaving only orientation-specific
   methods in the subclasses.

2. Unify pointsC1C2Horizontal/pointsC1C2Vertical

   These two methods had identical algorithmic structure with X/Y axes
   swapped. Replace with a single computeControlPoints(Qt::Orientation)
   method that parameterizes by axis.

3. Extract selectedItemsOfType<T> template helper

   selectedNodes() and selectedGroups() shared the same iteration
   pattern over QGraphicsScene::selectedItems() with qgraphicsitem_cast.
   Extract into a local template function.

https://claude.ai/code/session_015aZAxLSpakeTpF7zaq6D2F
Cleanup: dead code removal, bug fix, and deduplication
…ion, backward compat

- Upgrade Catch2 from v2.13.7 to v3.7.1 via FetchContent; remove test_main.cpp
- Add noexcept to trivial getters across NodeState, ConnectionState,
  ConnectionGraphicsObject, BasicGraphicsScene, ConnectionIdUtils, ConnectionIdIndex
- Encapsulate NodeStyle and GraphicsViewStyle public members behind getter
  methods, matching the pattern already used by ConnectionStyle
- Restore backward compatibility for legacy "intNodeId" JSON key in
  tryFromJson() connection deserialization

https://claude.ai/code/session_01PgU81XZwKqXaPr628mVdQj
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.

2 participants