Introduce an interpreter to extract command interpretation logic #211

sophiedeziel · 2024-10-09T00:19:29Z

This refactor is extracting responsibilities out of the parser and out of the webgl-preview class into an interpreter to produce job and paths primitives. The line used to be blurry in between the two classes. The interpreter is the missing piece that helps defining the boundaries in a clear way.

Key concepts

The parser

The parser's responsibility is only about converting a text file to an array of structured javascript objects that represents the list commands and their attributes. All the parser cares about is the syntax. It should never hold logic related to what the commands means and how they should be interpreted. If state has to be held based on the content of the gcode, it's a smell about code not belonging in there.

The interpreter

The interpreter takes the output of the parser. This is where the gcode commands are "implemented". It manages a virtual machine's state and generates a list of paths representing extrusions and travels. All it should care about is translating gcode into job and path primitives.

The Path

Represents a continuous travel of the tool, with specific attributes and type. It holds the coordinates and the logic to be rendered on a scene.

The job

Represents the state of a print or milling job. It holds info about current state and all the paths generated by the interpreter "running" the gcode on it.

The renderer

The webgl-preview class is the renderer. Maybe it should be renamed renderer? It takes the state of the virtual machine and the results from the interpreter and adds them to a 3D scene.

Next steps

This PR is still a work in progress and there are a lot of things I am not satisfied with yet.

For example, I've put 3D objects logic into the paths objects. It does not seem right to me. It makes the materials management overly complicated.
I'm also annoyed by the gcode enum for supported commands. It should be simpler to implement a new command.
For some reason, it is not very performant at the moment. This will be investigated after the refactor gets into a more stable state.

Todo

Reimplement the progressive rendering
Make sure GCode can be processed in chunks
Fix existing tests
~~Debug the CNC toolpath preset that does not render correctly~~ turns out it's an issue on develop, it was not introduced by the changes of this PR.
Extract out new features that creeped in (G21)
Extract out a PR for the demo fix for progressive rendering

Nice to have

Add tests for the Interpreter class
Add tests for the Path class
Add tests for the Job class

Not in scope for this PR

This PR is already large enough. Some shortcuts and less than ideal implementations were used to minimize the size of it. Listing them here as an acknowledgment of them and we'll open PRs later to address them individually.

Refactor to parse and interpret outside of the renderer
Improve the webgl-preview tests
Improve the methods filtering/looping through all commands or paths (Improve the paths subsets of jobs #220)
Improve the GCode Enum situation
Simplify parsing the GCode commands (g1/g01 and all the T)
Bring back layering split with tolerance

github-actions · 2024-10-09T00:20:44Z

Visit the preview URL for this PR (updated for commit 14b54ca):

https://gcode-preview--pr211-interpreter-c5zpoxi1.web.app

_{(expires Tue, 12 Nov 2024 15:01:07 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 59bd114ae4847b32c2bba0b68620b9069a3e3531}

src/gcode-parser.ts

src/interpreter.ts

src/machine.ts

src/path.ts

src/machine.ts

remcoder · 2024-10-10T06:40:56Z

Nice work @sophiedeziel! So many improvements!

Some dev process ideas that I want to throw out there:

most importantly I'd like to keep the diff for every PR not larger than needed. Focused PRs. This allows me for instance to pick up the PR later.
the main focus was to introduce the Interpreter. This could be not much more than moving code around since the logic is already present.
this means that other improvements really deserve there own PR.
smaller PRs are so nice because they create a more consistent flow of smaller improvements and prevent getting stuck on large changes.

At this point it would seem we can create a a separate PR for parser improvements. And another for splitting of a Renderer class.

And then there can be PRs for improving logic or perf or cleanup.

Most of this, except for the removal of layers, can all still be merged into V2.

I know we said we were ready for V3 but I think now that we would be in a better place if we tackle some things like this refactor first.

Your thoughts?

sophiedeziel · 2024-10-10T16:39:35Z

It is such a challenge to keep small! I agree with you that some performance stuff should be left for later. But there are some breaking changes that are not simply about moving code around, unless we introduce some of the classes in an unused state in parallel with the old code.

This PR should at a minimum be in a working state, which it is not 100%.

It should also include some minimal tests to help us in the next iterations.

Are you able to identify a part that could be it's own individual PR? That could be a start.

I'll scope out some of the todos, to prioritize mergability.

And how strongly do you feel about progressive rendering?

remcoder

This setup is a major improvement in many aspects. Indeed this is becoming a 'platform' we can build the next iteration of cool features on! I'm genuinely grateful for having such a brilliant contributor!! thank you for your hard work!

I did find some things that might deserve attention but nothing that stands in the way of merging this PR. I suggest your read through my remarks and then merge at will.

src/__tests__/interpreter.ts

src/interpreter.ts

remcoder · 2024-10-12T20:14:25Z

src/__tests__/path.ts

+  expect(result.parameters.lineWidth).toEqual(9);
+});
+
+test('.geometry returns an ExtrusionGeometry with the path line height', () => {


Honest question: what are you testing here? I would expect that this is covered by the type system. As long as we're using the type checker I don't think you end up breaking this. Or am I missing something?

I was wrestling with this test and other ones that are similar.

I wanted to make sure the geometry was passed the right attributes to the constructor. Sometimes it's valuable, sometimes it's not.

remcoder · 2024-10-12T20:39:29Z

src/webgl-preview.ts

@@ -403,13 +304,20 @@ export class WebGLPreview {
    this.initScene();

    this.renderLayerIndex = 0;
+
+    if (this.job.layers() === null) {


I believe you're executing job.layers() multiple times within a single render pass. You prob want to avoid that by just keep a local variable or instance prop _layers or something.

This is fixed in #220 🎉

ok nvrnd, I've seen the indexers PR now :-)

src/webgl-preview.ts

remcoder · 2024-10-12T20:45:47Z

src/webgl-preview.ts

    }
+
+    for (const color in this._geometries) {


This is very similar to what we have in v2. Current users already benefit from it!

* Interpreter prototype * arc support * units * Simplify toolchange * travelType * remove setInches * remove targetId * layers * render lines * extract machine * wip * Make it work with rerenders * Remove some layer references * Rename Machine to Job * Simplify parsing attributes * Am I going too far? * get rid of geometries once used * the geometries disposition is handled by the batchMesh * Fix tests * Bring back some code * Bring back progressive rendering * update dev-gui with job * Test Path * First interpreter tests * Test G0 and G1 * Test everything by G2 * Adding missing codes * Minimize diff on app.js * Leave G21 for a future PR * Job tests (and fixes!) * Keep G28 for a separate PR * Improve tests * Code improvements * Retractions are travel