Skip to content

dont need to flush fillets on transform #7204

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jessfraz
Copy link
Contributor

cleaning up the hack since #5880 is fixed

Copy link

vercel bot commented May 24, 2025

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

Name Status Preview Comments Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 24, 2025 1:36am

@jessfraz
Copy link
Contributor Author

hmm a test fails now so i guess we cant, not a huge deal but @benjamaan476 might find curious its scale_after_fillet

@jessfraz
Copy link
Contributor Author

but i know @benjamaan476 would rather i just remove all this delayed fillet nonsense insteaad ahhaha

Copy link

codspeed-hq bot commented May 24, 2025

CodSpeed Instrumentation Performance Report

Merging #7204 will not alter performance

Comparing do-not-flush-fillets-transform (df8ef79) with main (eb23278)

Summary

✅ 79 untouched benchmarks
🆕 1 new benchmarks
⁉️ 1 dropped benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
⁉️ parse_80-20-rail 56.4 ms N/A N/A
🆕 parse_t-slot-rail N/A 14.2 ms N/A

@benjamaan476
Copy link
Contributor

Could this become just executing the fillets when they appear in the kcl instead? I can't remember why they need to be delayed until later, but they might not need to be anymore?

@jessfraz
Copy link
Contributor Author

yeah 100% @benjamaan476 i knew you'd say that
stuff like this is why we save til the end #6954

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