Skip to content

Conversation

@Ayush2k02
Copy link

@Ayush2k02 Ayush2k02 commented Dec 27, 2025

Summary

Screen.Recording.2025-12-28.at.3.44.52.AM.mov

Fixes #3519

Instead of tracking PointIDs (which get remapped), we track point positions in the list. When Vector::concat() merges two layers, it preserves order:

  • Layer1's points stay at indices 0, 1, 2...
  • Layer2's points go to indices n, n+1, n+2... (where n = layer1's point count)

Below is a shorter version of the fix :

  1. Before merge: Save point indices (e.g., "point at index 1 in layer1, point at index 0 in layer2")
  2. Merge the layers
  3. After merge: Calculate new indices (layer2's index 0 becomes index 2 if layer1 had 2 points)
  4. Look up the actual PointIDs at those indices
  5. Create the segment using the resolved IDs
  6. This works because point ordering is preserved, even though IDs change.

@Ayush2k02 Ayush2k02 marked this pull request as draft December 27, 2025 01:04
@Keavon
Copy link
Member

Keavon commented Dec 27, 2025

Hello and thanks for the PR. I need to mention that we do not accept PR descriptions written by AI because they increase the noise-to-signal ratio and usually serve to mislead with obscured inaccuracies. (The same applies to code, which must be written by you.)

@Ayush2k02 Ayush2k02 marked this pull request as ready for review December 27, 2025 01:12
@Ayush2k02
Copy link
Author

Ayush2k02 commented Dec 27, 2025

Oh, hey , I had this extension turned on in my IDE https://whatthediff.ai/ , will be mindful to disable it when creating PRs in this repo :) . Also regarding using AI in code, I did use claude code for targeting the files and the fix but did review that intensively before publishing .

@Ayush2k02 Ayush2k02 changed the title fix: defer segment creation after merge to handle PointId remapping fix: joining paths from different layers with Ctrl+J fails Dec 27, 2025
@Keavon
Copy link
Member

Keavon commented Dec 27, 2025

Thanks for the insight into your process (we've been seeing more highly-elaborate-but-not-insightful PR descriptions like this and it's good to know some of the details about what kinds of AI tools are producing them). For code authorship, AI is acceptable for auto-completing lines you would already be typing, but we need contributions to always come from the original thought processes of its human authors who can answer to the reasoning behind each line of changes. Code can't be mysteriously changed without anyone aware of why the change was made. Thanks for working to follow our requirements to building an effective software project.

Copy link
Contributor

@0HyperCube 0HyperCube left a comment

Choose a reason for hiding this comment

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

It doesn't work. I have tried it with some simple lines with a transform.

I guess the vibes haven't fully completed the code.

fail_to_join.graphite

cannot_join_lines_with_transform.mp4

@Ayush2k02
Copy link
Author

Taking a look

@Ayush2k02 Ayush2k02 marked this pull request as draft December 27, 2025 20:26
When using Ctrl+J to join two points from different layers, the operation
failed because PointIds change during merge due to collision resolution.

The fix uses index-based point tracking instead of PointIds:
1. Before merge: Record the index of each selected point in its layer
2. Calculate post-merge indices using the point offset from layer1
3. After merge: Retrieve the actual PointIds at those indices
4. Create the connecting segment using the resolved PointIds

This works because Vector::concat() preserves point ordering during merge,
so we can use exact index arithmetic to find points after remapping.

Fixes GraphiteEditor#3519
@Ayush2k02 Ayush2k02 force-pushed the fix/joining-paths-same-pointids branch from 27f0b7b to d604ef6 Compare December 27, 2025 22:04
@Ayush2k02 Ayush2k02 marked this pull request as ready for review December 27, 2025 22:16
@Ayush2k02
Copy link
Author

Ayush2k02 commented Dec 27, 2025

After taking a second look at the issue, here is what I came up with: The fix uses index-based tracking to handle PointId remapping:

  • Vector::concat() preserves point ordering (layer1 points first, then layer2 points)
  • Save each point's index before merge
  • Calculate post-merge indices using layer1's point count as offset
  • Look up actual PointIds at those indices after merge

Other approaches I considered:

  • Position-based matching: Save coordinates, search within 250px tolerance after merge. Fragile—Flatten Path shifts coordinates unpredictably, large tolerance risks wrong matches.
  • Hash-based ID prediction: Replicate collision resolution to predict new PointIds. Impractical—hash seed needs internal parameters (current_index, vector_index, node_id) inaccessible from shape_editor.rs.

@Ayush2k02
Copy link
Author

Let me know if I am missing something or if there could be any other edge case here

@Ayush2k02 Ayush2k02 requested a review from 0HyperCube December 27, 2025 22:31
@Ayush2k02
Copy link
Author

Screen.Recording.2025-12-28.at.7.12.43.AM.mov

More manual testing with data grid for when the conflicting PointId is not updated so the wrong segment is created , here it is not created .

Copy link
Contributor

@0HyperCube 0HyperCube left a comment

Choose a reason for hiding this comment

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

fail_with_repeat.mp4

However I guess it doesn't matter too much.

@Ayush2k02
Copy link
Author

Ayush2k02 commented Dec 28, 2025

Taking a look , also agreed , you are good at finding these edge cases :)

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.

Joining paths with the same PointIds doesn't work

3 participants