Skip to content

Conversation

@0HyperCube
Copy link
Contributor

@0HyperCube 0HyperCube commented Oct 30, 2025

Closes #3318

@Keavon Keavon changed the title Stable segment ids in the spline node Fix the Spline node to maintain stable segment IDs Oct 30, 2025
@TrueDoctor
Copy link
Member

I lack the mental model to judge if this is a good approach. Could this lead to cases where we merge lists of segments with the same ids? And would that create issues? If the change is conceptually sound, the code looks good, I just don't know how we would expect this to behave

@0HyperCube
Copy link
Contributor Author

@TrueDoctor

The behaviour of producing small incrementing segment IDs is used by other nodes that generate geometry such as the circle and rectangle nodes.

In the flatten_path node, if there is a duplicate segment ID, it will deterministically modify one of the IDs through hashing (current_index, vector_index, node_id) and the previous segment ID:

let mut hasher = DefaultHasher::new();
(current_index, vector_index, node_id).hash(&mut hasher);
let collision_hash_seed = hasher.finish();
output.element.concat(other, transform, collision_hash_seed);

Then generating the new IDs in the concat function:

let segment_map = additional
.segment_domain
.ids()
.iter()
.filter(|id| self.segment_domain.ids().contains(id))
.map(|&old| (old, old.generate_from_hash(collision_hash_seed)))
.collect::<HashMap<_, _>>();

This means that segment IDs must be:

  • Unique within the current SegmentDomain.
  • Deterministic between several runs of the same graph.

Copy link
Member

@TrueDoctor TrueDoctor left a comment

Choose a reason for hiding this comment

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

Alright, this looks good then, thanks for the detailed explanation @0HyperCube

@TrueDoctor TrueDoctor enabled auto-merge (squash) October 31, 2025 11:17
auto-merge was automatically disabled October 31, 2025 13:14

Head branch was pushed to by a user without write access

@0HyperCube 0HyperCube force-pushed the stable-spline-segment-ids branch from 6f629ee to a18089a Compare October 31, 2025 13:14
@TrueDoctor TrueDoctor merged commit 7c28a53 into GraphiteEditor:master Oct 31, 2025
4 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.

Path handles glitch out when moved

2 participants