feat: openscad to js compiler package added#1725
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1725 +/- ##
==========================================
- Coverage 95.34% 94.62% -0.72%
==========================================
Files 37 44 +7
Lines 8329 9441 +1112
==========================================
+ Hits 7941 8934 +993
- Misses 388 507 +119 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
generated files should be in .gitignore |
| compile_all_examples.ts | ||
| examples/ | ||
| images/ | ||
| out/ |
There was a problem hiding this comment.
/src layout looks out of date. I think it might also be nice to move examples, images, and out under a test directory, since I believe that's what they are?
There was a problem hiding this comment.
In addition, I think the whole shebang should either go under bindings/wasm/examples. Maaaybe bindings/wasm/lib, but I see this as a different layer.
There was a problem hiding this comment.
@elalish I changed the layout from /src to flat layout keeping all the things in root of the compiler which were inside the /src. Also moved the out, examples inside /test directory and added /test/out in .gitignore as per @pca006132 suggested. And the images are actually for the comparison.md file, should that also be moved to test directory?
@tonious Earlier I had a conversation with John, he suggested to keep this inside /bindings/wasm/openscad-compiler as a separate package. Should I move it to any other directory or keep it as it is?
| "commander": "^14.0.3", | ||
| "express": "^5.2.1", | ||
| "manifold-3d": "^3.3.2", | ||
| "puppeteer": "^24.38.0" |
There was a problem hiding this comment.
puppeteer is kind of a heavy dependency. Where is it used?
There was a problem hiding this comment.
Actually, I was trying to write tests the compiled outputs programmatically in a node js environment, that's why added puppeteer and express. I have removed those as of now.
There was a problem hiding this comment.
Sure, I will add those as dev dependency when I write the tests.
There was a problem hiding this comment.
Puppeteer is really heavy for tests even. It should be possible to implement tests without in-browser tests. It's a compiler -- it should run fine in node.
There was a problem hiding this comment.
Sure, I will try to do so. Actually, previously I was trying to test if the generated code runs on the viewer.html without any error or not, so I used Puppeteer. I will try to write tests for checking the correctness in a different way.
…inside /test directory
…l_examples.ts files
|
This is awesome! But it's also a huuuge chunk of code. I'm posting comments as I think of them, so they may be disjoint. I'm not trying to be difficult. I mean, I might be difficult, but I'm not trying to be. My apologies in advance. My comments will typically come from a few specific concerns:
|
|
IMHO, |
|
@tonious Thanks for the feedback! Those concerns are completely fair for a PR of this size. I'll work on making the structure and flow easier to follow, and split things up where it makes sense. I'll also add more docs and tests to improve maintainability. For now, I've moved the |
|
Today's thoughts:
I'm wrapping my head around the server/viewer thing a little bit. My feeling is that it's extraneous, but that is an opinion as yet unsubstantiated by any thought process. |
…ve script in package.json
|
… surface compiling feature
…tion_for module, fixed polyhedron and rotate_extrude implementation
…ntation according to the special variables; added source file tracking and emitting at the compiled code as comments;
…ping and special variable scoping; added overwrite feature for premitives and other minor fixes
…ock, fixed trigonometric calculations, fixed semantic issues
|
I think you may be running openscad in multithreaded mode while our js is only single threaded. You can try to limit the number of threads to openscad. On linux you can do it with |
I am on windows and I think for OpenSCAD on Windows it is compiled without TBB (Threading Building Blocks) support (I am not quite sure about that but I asked this to several LLMs and they all told this). I tried running openscad, restricting it to single core (using CPU affinity bitmask) and the way I am running now, the time it took to compile is almost same. I have done it for several files and pasted output for four files - PS D:\manifold\bindings\wasm\openscad-compiler> .\threading_check.ps1
Running normally
Normal Internal Timer: Total rendering time: 0:00:00.149
Running locked to CPU Core 1
Restricted Internal Timer: Total rendering time: 0:00:00.172
PS D:\manifold\bindings\wasm\openscad-compiler> .\threading_check.ps1
Running normally
Normal Internal Timer: Total rendering time: 0:00:05.255
Running locked to CPU Core 1
Restricted Internal Timer: Total rendering time: 0:00:05.806
PS D:\manifold\bindings\wasm\openscad-compiler> .\threading_check.ps1
Running normally
Normal Internal Timer: Total rendering time: 0:00:00.022
Running locked to CPU Core 1
Restricted Internal Timer: Total rendering time: 0:00:00.028
PS D:\manifold\bindings\wasm\openscad-compiler> .\threading_check.ps1
Running normally
Normal Internal Timer: Total rendering time: 0:00:00.010
Running locked to CPU Core 1
Restricted Internal Timer: Total rendering time: 0:00:00.016here is the script I am running - $file = 'test/examples/tree.scad'
$exe = 'C:\Program Files\OpenSCAD (nightly)\openscad.exe'
Write-Host "Running normally"
$p = Start-Process $exe -ArgumentList "-o normal.off --backend=manifold $file" -NoNewWindow -RedirectStandardError normal_err.txt -PassThru
$p.WaitForExit()
$normalRenderLine = Get-Content normal_err.txt | Where-Object { $_ -match "Total rendering time:" }
Write-Host "Normal Internal Timer: $normalRenderLine"
Write-Host "Running locked to CPU Core 1"
$p = Start-Process $exe -ArgumentList "-o affinity.off --backend=manifold $file" -NoNewWindow -RedirectStandardError affinity_err.txt -PassThru
$p.ProcessorAffinity = 2
$p.WaitForExit()
$affinityRenderLine = Get-Content affinity_err.txt | Where-Object { $_ -match "Total rendering time:" }
Write-Host "Restricted Internal Timer: $affinityRenderLine"
Remove-Item normal.off, affinity.off, normal_err.txt, affinity_err.txt -ErrorAction SilentlyContinueAm I doing it right? |
|
I have no idea about powershell, so cannot really say anything about your script. However, I am pretty sure openscad on windows is compiled with tbb. The simplest way to check this would be to run openscad on a non-trivial model (high resolution sphere union) and check CPU utilization yourself. And well, I tried asking gemini and it agrees with me too, so I cannot be wrong :P
|
|
My mistake. Indeed openscad on windows is compiled with tbb. I just verified that. I am now running openscad in wsl and with taskset to limit the openscad to run on single threaded mode. I've rewritten the benchmark script. Now the data I am getting are -
Speedup Comparison Chart (in log scale)
Time Distribution Chart
|
|
Timing distribution graph: Nice plot, but not very useful, since I can't read the proper total time here and compare it with openscad's time. This kind of cumulative plot works when the timings are components of a total time, e.g., compile and run of our js output, but openscad's timing is not one component of the total time. Try to change it that way. Speedup comparison graph: Also not very useful, I have no idea what the axis actually means. Also, I don't think it is an appropriate use of log scale, things are not different in scale. The bigger question here is, why are we so much slower even when openscad is running in a single thread? Geometry evaluation? Other stuff? |
|
@pca006132 Actually the previous benchmark result was incorrect due to some WSL issues. Here is the corrected data -
Time Comparison Chart
So most of the time the OpenSCAD to TS compilation + the time to run TS file lesser than the time to compile OpenSCAD. I will prepare more decorated stats by categorizing these files according to what they test for. |
|
Nice plot! That looks excellent and as expected. Good work. |
|
Nice. Here are some follow up that we can look into later (not urgent, fix the failing tests first)
|
…ment handling for explicit undef, improved linear_extrude and rotate semantics, fixed range iteration edge cases
|
I have pushed the fixes. Now all the test cases are passing. Here is the benchmark script - https://github.com/swaparup36/manifold/blob/openscad-to-manifold/bindings/wasm/openscad-compiler/test/benchmark.ts For openscad I am measuring the time that openscad takes to compile and export the openscad code to .off file, and openscad is running it on single core. I will do the separation of the logic part and geometry part by csg tree and check how much it takes for the logic part. And see why for some files the compiled one is slower. |
Doesn't look like all the tests are passing yet on the CI. |
…est configuration file
…produces the output before running
Yes, that is because of some dependency of the compiler on external resources like fonts, images and other openscad libraries. First I thought that the tests are not passing because the output files are not pushed along with the examples, so I made the tests generate the output files before running the tests. But it is still not working because many of the openscad example taken here is dependent on some external fonts, images, libraries, so when these files are compiled the compiler takes care of these dependencies and generates runtime exports. These dependencies are in my local machine and compiler can find it by env variables, but the CI environment does not have those locally. Moreover the semantic test runs the example using openscad cli that is not present on the CI environment as well. The fix I am thinking for this - |
|
Yes, I think we should not have openscad binary in our CI. Looking at the CI failure though, I think you can address the simpler ones first, e.g., format your code, make sure the tests can find the runtime.js file, etc. |
Sure! I guess the runtime.js is not available because it is not built before running tests. I will add that before the tests run. |
|
I have written a script to separately measure the time taken by the logic and geometry part for the bosl2 files. Here are the results -
|







No description provided.