Refactor Fillet Logic to Combine Multiple Tags Within the Same Fillet Expression #4058
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
max-mrgrsk
added
refactor
ast
Irev-Dev
reviewed
Oct 2, 2024
| let pathToFilletNodes: Array<PathToNode> = [] | ||
| // Step 1: modify ast with tags and group them by extrude nodes (bodies) | ||
| const extrudeToTagsMap: Map<PathToNode, string[]> = new Map() | ||
| const lookupMap: Map<string, PathToNode> = new Map() // work around for Map key comparison |
There was a problem hiding this comment.
I found this a little confusing, but I get what's going on here.
…4039-group-all-fillet-tags
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this issue is part of Fillet project: #2606
continues AST mod: #3680
closes: #4039
PR Description:
Overview:
This PR refactors the
modifyAstCloneWithFilletAndTaglogic to consolidate multiple edge selections into a single fillet expression per extrusion, combining tags for all selected edges into one operation. The changes streamline the handling of multiple selections and remove redundant code that was previously designed for single selections.Key Changes:
1. Two-Step Looping Logic:
extrudeKey, ensuring that tags related to the same extrusion are grouped together.2. Removal of
addFilletFunction:The
addFilletfunction, which was originally designed to handle single edge selections, became redundant after we refactored it's logic intomodifyAstCloneWithFilletAndTagloops. With the refactor,addFilletis no longer needed and has been removed.3. Test Suite Consolidation:
Previously, unit tests for single selections were separate from tests for multiple selections (which required mocking
kclManager.executeAst({ ast })due to the use of the artifact graph). This PR consolidates the test suite, combining single and multiple selection tests into the same framework.Outstanding Issues (To Be Addressed in a Separate PR):
1. Support for Fillet in Sketch Pipes:
The current implementation does not handle cases where the
extrudeis part of the sketch pipe. This limitation will be addressed in a future update.2. Handling Already-Filleted Edges:
If an edge that has already been filleted is selected again, the current logic does not remove or modify existing fillets. A future PR will address this by checking for existing fillets and removing the tags from previous fillets before applying new ones.
related comments: