Repository navigation
Conversation
3140430 to
ce03443
Compare
ce03443 to
8c08238
Compare
8c08238 to
d9d4478
Compare
|
The Windows exe workflow is not failing to build. Its The failureAll six are Failing: This is a value difference, not a formatting one
I re-ran the file locally on Linux: 15 passed. So the goldens are correct here and were not committed stale — the divergence is real and it is in the placement/offset arithmetic, not in the tests. 0.005 mm is far too large to be float noise in a single operation, which makes me think something in the offset or inset path sits near a tolerance and lands on the other side of a branch on Windows. Worth a look on its own, quite apart from this PR: if job placement is not bit-reproducible between platforms then the placement outline shown in the UI and the coordinates actually sent to the machine can disagree slightly depending on where rayforge was built. Options for the testsComparing G-code by exact string equality will keep biting here for as long as the numbers are not reproducible. Two ways out:
My suggestion is (1) to get this PR green and (2) as its own follow-up, since (2) should not hold up a feature that is otherwise passing everywhere else. I have not run the executable on Windows and cannot — I have no Windows machine or licence, so my confirmation here is the CI log plus the local Linux run above. |
The SVG trace strategy rendered at sqrt(VTRACER_PIXEL_LIMIT) = 1104 px. With the 2 px border the tracer adds, the image handed to vtracer is 1108x1108 and exceeds the limit, so _handle_oversized_image downscaled it - by a different factor per platform, because the limit was platform-dependent (1_220_000 on Linux, 1_000_000 on Windows). The resulting traced geometry differs by about 0.005 mm, which made the job-origin golden G-code tests fail on the Windows build. Use the conservative 1_000_000 limit on every platform, and size the SVG render to fit under it including the border so that tracing no longer downscales at all. Regenerate the job-origin goldens: they now carry the exact 0.05 mm stroke half-width instead of a downscale artifact. Refs barebaric#538
|
Thanks for the PR! I fixed the failing test so in theory this could be ready to merge. However, this PR worries me because it touches on one of the most critical and error prone pieces of key functionality, and there are many special cases to consider:
So before merging this to main I would like to see real user feedback - IMO this one requires careful testing on more than just one machine type and use case. |
|
Thanks for the tracing fix, and fair point: this touches the most critical path, so it should not land on the strength of a single bottom-left test machine. I can't provide real-machine feedback for other machine types either. What I could do was trace each special case through the code, pin it with tests, and make every combination I couldn't make correct refuse the job with an explanation instead of guessing. Pushed on top of your commit:
Absolute Coordinates (the default) is byte-identical to main. I generated the expected G-code on the merge base
Still open (not covered by the tests above):
I'm fine with this staying unmerged until people with other machines have tried the branch. Alternatively it could be merged behind a setting (e.g. an "experimental" toggle that hides the Start From rows and forces Absolute). Do you want it flagged as experimental that way, or kept on the branch until there is user feedback? |
A document now carries a job origin: the Start From mode (Absolute Coordinates, Current Position or User Origin) and one of nine anchor points on the job's bounding box. It is saved with the project and older projects load as Absolute Coordinates. job_placement resolves it against a machine: User Origin is the zero point of the active WCS (or the work area origin), Current Position is the head position the controller reports (the pointer dot while pointer alignment is on) and needs a connected, idle machine. Refs barebaric#518
The job origin is applied as one rigid XY translation of the whole job in world space, folded into the workpiece placement of the step aggregates. Everything downstream (machine transform, encoder, 3D preview, framing, sanity checks) sees the placed job, while the G-code stays absolute in the active WCS: no G92 or G10 is emitted and nothing is written to the controller. - The anchor refers to the bounding box of the designs on the canvas (job_design_rect), not the toolpath with overscan or lead-ins. - Current Position re-reads the head position for every job request, so send, frame and export never reuse a stale placement. - Rotary layers only support Absolute Coordinates. - MachineCmd refuses a placed job that leaves the machine extents or enters a no-go zone, and in Current Position mode moves the head back to the start position after framing or a finished job, because the dialect postscript usually returns to X0 Y0. - workpiece_world_aabb used the size on top of the world transform, which already scales the unit square, so workpieces without vector geometry (images) got a far too large box. Golden G-code tests cover every mode on GRBL. Refs barebaric#518
The Control Panel gets a Start From row and a 3x3 Job Origin selector under the WCS selector; changes are undoable and the subtitle shows where the job will start. When the mode is not Absolute, the 2D canvas draws a dashed outline of the placed job with a marker on its origin point, following the head in Current Position mode. Refs barebaric#518
The pre-flight check reused the cached job, which in Current Position mode was placed where the head was when it was assembled. It now assembles the job afresh in that mode. Refs barebaric#518
The SVG trace strategy rendered at sqrt(VTRACER_PIXEL_LIMIT) = 1104 px. With the 2 px border the tracer adds, the image handed to vtracer is 1108x1108 and exceeds the limit, so _handle_oversized_image downscaled it - by a different factor per platform, because the limit was platform-dependent (1_220_000 on Linux, 1_000_000 on Windows). The resulting traced geometry differs by about 0.005 mm, which made the job-origin golden G-code tests fail on the Windows build. Use the conservative 1_000_000 limit on every platform, and size the SVG render to fit under it including the border so that tracing no longer downscales at all. Regenerate the job-origin goldens: they now carry the exact 0.05 mm stroke half-width instead of a downscale artifact. Refs barebaric#538
Absolute Coordinates is the default and must not change any existing setup. The expected G-code was generated on the merge base before Start From existed (c9ac566) for a bottom-left bed, top-right with negative axes, top-left and bottom-right with "machine origin is work area origin" and margins, a layer with its own WCS, and a rotary layer. The default job origin must reproduce it byte for byte. Refs barebaric#518
- The Job Origin is picked on the canvas, but was applied in world space. On a rotated panel "top left" therefore placed another corner of the job on the start point. Map the anchor through the panel rotation, for the G-code and for the outline marker. - A layer with its own WCS is emitted relative to another zero point than the one the job is placed on. Refuse User Origin and Current Position for such layers (unless the work area origin is the zero point, where WCS offsets are not used), as for rotary layers. - Show why Start From is not available in its subtitle, and hide the placement outline instead of drawing one for a refused job. - Applying a Print & Cut alignment places the design on the printed marks; switch Start From back to Absolute Coordinates with a notice so the job is not moved away from them. Pin Start From on every origin corner, with and without negative axes and with "machine origin is work area origin" plus margins: User Origin and Current Position G-code, the preview placement, and framing with the return to the start position. Refs barebaric#518
173383a to
9314c2e
Compare
knipknap
left a comment
knipknap
left a comment
There was a problem hiding this comment.
Ok, I tried to give this one a deeper review, but I am sorry that I gave up - this has severe architectural layer violations to the point that I have a hard time understanding what happens or explaining it properly. Some examples include: decentralizes coordinate space conversions, geometry math in the pipeline, artifacts from past refactoring, and other code quality issues. I am sorry but this is not mergeable.
If a feature like this lands, I would recommend going in tiny, iterative steps - starting from the core, e.g. introduce the new coordinate system with strictly defined terminology and well documented central conversion function first (which probably would already require some discussion), proving equivalence, then extending the data model, ... or something like this.
This PR touches several critical code paths at once, which would make any regressions extremely hard to pin down.
| self._editor.notification_requested.send( | ||
| self, | ||
| message=_( | ||
| "Start From was set to \u201cAbsolute Coordinates\u201d " |
There was a problem hiding this comment.
Please do not use \ sequences for UTF chars; all modern versions of Python support UTF8 natively
| from raygeo.geo.types import Point | ||
|
|
||
| # A world-space bounding box as (min_x, min_y, max_x, max_y). | ||
| BBox = tuple[float, float, float, float] |
There was a problem hiding this comment.
Raygeo has a Rect type for this
| artifact_store.release(handle) | ||
| raise | ||
|
|
||
| def _start_position_in_command_space( |
There was a problem hiding this comment.
Coordinate space conversion is responsibility of coordspace.py
|
Thanks for taking the time for a deep review, and fair enough: this touched too many critical paths at once, and the coordinate handling isn't where it belongs. I'm closing it. If the feature comes back, it will start small, as you suggest: first an issue to agree on the coordinate-system terminology and a single, documented conversion function, then a PR that only adds that and proves the output is unchanged, and the data model and UI after that, one step at a time. |
This branch was successfully deployed
2 participants
Summary
Add a configurable Start From for laser jobs: Absolute coordinates
(default), Current Position, or a User Origin, with a placement outline on the
work surface so you see where the job will be cut.
Fixes #518.
What's in this PR
Position / User Origin and one of 9 anchor points (corners + edges + center).
active, a blue dashed rectangle shows where the job will land, following the
head when "Current Position" is selected.
outline and jobs are placed as before.
selected origin.
Overview
The job origin is a document-level property (persisted in the project file),
so the same choice applies to every job prepared from the document.
Tests
tests/core/test_job_origin.py— model, defaults and roundtrip.tests/machine/test_job_placement.py— placement math for the 9 anchorsand both non-absolute modes.
tests/pipeline/test_job_origin_gcode.py— G-code reflects the origin.tests/ui_gtk/canvas2d/test_job_placement_overlay.py— outline visibilityand follow-the-head behaviour.
tests/pipeline/test_job_design_rect.py— design rect used for placing.All targeted tests pass (
pytest tests/core/test_job_origin.py tests/machine/test_job_placement.py tests/pipeline/test_job_origin_gcode.py tests/pipeline/test_job_design_rect.py→ 56 passed).