Add YOLO (Recommended) / YOLO (Perfectionist) flow-ratio calibration methods - #2053
Merged
Merged
Conversation
…thods Closes #2051 - Add FlowRateYoloRecommended/FlowRateYoloPerfectionist to CalibrationMethod enum with wire names, DefaultModelFileName, and RelativeResourcePath matching upstream OrcaSlicer's Orca-LinearFlow.3mf/Orca-LinearFlow_fine.3mf resources (filament_flow/ directory). - Guard OrcaSlicingPipelineService.PrepareCalibrationModel to fail loudly for the two new methods instead of silently mis-slicing: their bundled 3MF resources encode per-object flow ratios as baseline-relative deltas (flowrate_0.01, flowrate_m0.01), incompatible with the existing FlowRateCalibrationConfigurator parser built for absolute percentages (flowrate_95). A delta-aware configurator is follow-up work. - Document investigation findings in CalibrationMethod.cs remarks: max_volumetric_speed/retraction are not blocked upstream, simply not yet built here (contrast with PA Pattern/PA Line's real GPL-3.0 provenance blocker). - Add test coverage: TryParse acceptance/rejection for the new wire names, DefaultModelFileName/RelativeResourcePath, and a guard test asserting PrepareCalibrationModel throws for both new methods.
Address review findings (Hicks, Vasquez): - Add CalibrationMethods.IsSlicerSupported(), and reject FlowRateYoloRecommended/FlowRateYoloPerfectionist in SliceJobController before a job is ever queued, instead of only failing late in OrcaSlicingPipelineService.PrepareCalibrationModel after a worker has already claimed the job (Vasquez: fail-late robustness/DoS concern). - Assert the worker guard's exception message content, not just its type, so a test failure actually catches a regression in the fail-loud contract (Hicks). - Add controller-level test coverage confirming the two YOLO methods are rejected with calibration_method_not_yet_supported before AddAsync/ NotifyJobQueuedAsync are ever called.
Bishop's round-2 review found that CalibrationMethods.SupportedWireNames (used to build the 'unsupported_calibration_method' error message) still listed the two YOLO wire names, even though submitting them is rejected with a different error (calibration_method_not_yet_supported). A client hitting an unrelated unsupported-method error would be told those two methods are supported, when they are not. Add CalibrationMethods.ClientAcceptedWireNames: the subset of SupportedWireNames for which IsSlicerSupported is true. Use it in SliceJobController's 'Supported methods' error text instead of SupportedWireNames, which keeps its original meaning (every wire name TryParse/ToWireName round-trip against) for that invariant's test. Add regression coverage: a controller test asserting the YOLO wire names never appear in the unsupported-method error's supported list, and a catalogue-level test asserting ClientAcceptedWireNames excludes exactly the two YOLO methods.
TryParse's XML doc still said an unrecognized-or-not-yet-supported name returns false, but the two YOLO methods are intentionally catalogued and parse successfully (true) despite not being slicer-supported yet. Update the doc to describe the actual two-tier contract: TryParse gates the catalogue, IsSlicerSupported/ClientAcceptedWireNames gate client submission.
github-actions
Bot
added
the
squad
Collaborator
Author
|
Squad-Reviewer: bishop |
Collaborator
Author
|
Squad-Reviewer: hicks |
Collaborator
Author
|
Squad-Reviewer: vasquez |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| CalibrationMethod.FlowRatePass1 => Path.Combine("filament_flow", "flowrate-test-pass1.3mf"), | ||
| CalibrationMethod.FlowRatePass2 => Path.Combine("filament_flow", "flowrate-test-pass2.3mf"), | ||
| CalibrationMethod.TemperatureTower => Path.Combine("temperature_tower", "temperature_tower.drc"), | ||
| CalibrationMethod.FlowRateYoloRecommended => Path.Combine("filament_flow", "Orca-LinearFlow.3mf"), |
| CalibrationMethod.FlowRatePass2 => Path.Combine("filament_flow", "flowrate-test-pass2.3mf"), | ||
| CalibrationMethod.TemperatureTower => Path.Combine("temperature_tower", "temperature_tower.drc"), | ||
| CalibrationMethod.FlowRateYoloRecommended => Path.Combine("filament_flow", "Orca-LinearFlow.3mf"), | ||
| CalibrationMethod.FlowRateYoloPerfectionist => Path.Combine("filament_flow", "Orca-LinearFlow_fine.3mf"), |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2051
Summary
OrcaSlicer's flow-ratio calibration dialog offers 4 methods (
Pass 1 (Coarse),Pass 2 (Fine),YOLO (Recommended),YOLO (Perfectionist)), butCalibrationMethod.csonly catalogued 3. This adds the two missing YOLO methods to the catalogue, matching the existing wire-name / enum /DefaultModelFileName/RelativeResourcePathpattern, while explicitly guarding against silently mis-slicing them until a follow-up delta-aware configurator exists.What changed
CalibrationMethod.cs: addedFlowRateYoloRecommended/FlowRateYoloPerfectionistenum members, wire names (flow_rate_yolo_recommended,flow_rate_yolo_perfectionist),DefaultModelFileName/RelativeResourcePath(Orca-LinearFlow.3mf/Orca-LinearFlow_fine.3mfunderfilament_flow/), and a newCalibrationMethods.IsSlicerSupported(method)gate (falsefor the two YOLO methods).SliceJobController.SubmitAsync: afterTryParsesucceeds, rejects the two YOLO methods immediately (400,calibration_method_not_yet_supported) before the job is queued — so a request that can never succeed doesn't consume a worker slot or job-record lifecycle. Also addedCalibrationMethods.ClientAcceptedWireNames(the catalogue minus not-yet-supported methods) so the "Supported methods" text in the unrelatedunsupported_calibration_methoderror never advertises a method the API will reject.OrcaSlicingPipelineService.PrepareCalibrationModel: added an explicitInvalidOperationExceptionguard for the two YOLO methods (defense-in-depth beyond the controller-level rejection), with a message that doesn't leak internal filesystem paths.CalibrationTests.csandSliceJobControllerCalibrationTests.csfor catalogue entries,TryParseaccept/reject, the client-accepted-list filter, the controller rejection ordering (beforeAddAsync/NotifyJobQueuedAsync), and the worker guard's message contract.Why not full support yet
OrcaSlicer's YOLO 3MF resources use a delta-based per-object naming scheme (
flowrate_0,flowrate_0.01,flowrate_m0.01, …) that's incompatible with the existingFlowRateCalibrationConfigurator, which is built for pass1/pass2's absolute-percentage scheme (flowrate_95,flowrate_102). Wiring YOLO into the existing configurator would silently mis-scale or skip objects rather than fail loudly. Building a correct delta-aware configurator is a separate, larger piece of work — out of scope for this issue, which only asks for the catalogue. Until that lands, submitting these methods is rejected with a clear error rather than silently producing corrupted G-code.Investigation findings (per issue ask — not implemented)
Plater::calib_max_vol_speed,Plater::calib_retraction). Nothing upstream blocks them — they're simply not yet built in this repo (no configurator/pipeline wiring here, unlike flow-rate/temperature-tower). Not a licensing or upstream blocker.CalibrationMethod.cs's remarks (not touched/resolved here — expanded the comment to keep this decision durable, not chasing a separate resolution).src/api/Services/Calibration/CalibrationMethodNames.cs, 12 methods, different wire-name convention): confirmed dead code from the removed calibration-projects saga (its own doc comment references deleted D7/D8 generator subtree dependents). Not reachable from the slice pipeline. Left untouched, as instructed.Validation
dotnet build ./farm-web.sln— 0 errors.Farm.OrcaSlicer.Worker.Tests.CalibrationTests+Farm.Slicer.Module.Tests(SliceJobControllerCalibrationTests) — all passing.dotnet format ./farm-web.sln --verify-no-changes— clean on all changed files.origin/developmentinto the branch; re-ran targeted tests on the merged result.Review
Reviewed by Bishop, Hicks, and Vasquez per repo convention — all three reached unanimous APPROVE at head
d0d0a26143d1ddaac75d7556b347534bfbda446eafter two rounds of fixes (see verdict comments below).