Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bring back multi profile #4828

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Bring back multi profile #4828

wants to merge 23 commits into from

Conversation

Irev-Dev
Copy link
Collaborator

@Irev-Dev Irev-Dev commented Dec 17, 2024

Revert revert.

closes #4828
closes #4619
closes #5191

Copy link

vercel bot commented Dec 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview Jan 10, 2025 8:50pm

Copy link

qa-wolf bot commented Dec 17, 2024

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

Comment on lines 1134 to 1135
/** {@deprecated} this information should come from the ArtifactGraph not digging around in the AST */
function getWallOrCapPlaneCodeRef(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the dodgy thing @jtran that we can look to remove, it's used to get the codeRef for startSketchOn in the case of startSketchOn(extrude, faceTag), but we can remedy with more info returned from execution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Irev-Dev
Copy link
Collaborator Author

@nadr0 e60cabb is where I "fix" the bad UX by changing the order such that the Xstate event is fired without waiting for the file write.

But ofc feel free to change if you think we can do better.

@Irev-Dev
Copy link
Collaborator Author

Irev-Dev commented Dec 17, 2024

e2e test failures don't seem related.
And they were passing the first time this was merged.

* Add Rust side artifacts for startSketchOn face or plane

* move ast digging

---------

Co-authored-by: Kurt Hutten Irev-Dev <k.hutten@protonmail.ch>
@Irev-Dev Irev-Dev force-pushed the kurt-bring-back-multi-profile branch from bddf028 to 0642e49 Compare December 20, 2024 12:21
@Irev-Dev
Copy link
Collaborator Author

I resolved conflict (might have made mistake) but tests are still failing.

@pierremtb
Copy link
Collaborator

Triggered CI after new snaps

Copy link

codecov bot commented Dec 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.84%. Comparing base (73a7e2b) to head (30397ba).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4828   +/-   ##
=======================================
  Coverage   85.84%   85.84%           
=======================================
  Files          88       88           
  Lines       31429    31432    +3     
=======================================
+ Hits        26981    26984    +3     
  Misses       4448     4448           
Flag Coverage Δ
wasm-lib 85.84% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jtran
Copy link
Collaborator

jtran commented Jan 9, 2025

I just merged main and naively fixed merge conflicts. But there's a tsc error. Some new code needs to be updated.

I also blew away PNG snapshot changes that conflicted and used the ones from main. I figured we can let them be regenerated.

@lf94
Copy link
Contributor

lf94 commented Jan 9, 2025

All tests are now passing; onto I guess just playing around a bit

@lf94
Copy link
Contributor

lf94 commented Jan 9, 2025

Ok, there are still some rough cases:

@lf94
Copy link
Contributor

lf94 commented Jan 9, 2025

2025-01-09_16-29-00.mp4

@lf94
Copy link
Contributor

lf94 commented Jan 10, 2025

Found another issue, doesn't highlight old sketch code:

2025-01-10_11-46-20.mp4

@lf94
Copy link
Contributor

lf94 commented Jan 10, 2025

Found another issue, it's selecting multiple sketches that do not belong to each other:

2025-01-10_11-49-01.mp4

@@ -195,7 +195,7 @@ export const ModelingMachineProvider = ({
store.videoElement?.pause()

return kclManager
.executeCode()
.executeCode({ isPartialExecution: true })
Copy link
Collaborator

Choose a reason for hiding this comment

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

With #5005, I think we determined this isn't partial execution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants