Skip to content

Conversation

@wawanbreton
Copy link
Contributor

This restores the previous behavior which was broken during scarf seam implementation. As a bonus, it also improves the display so that it now properly shows the actual spiral moving up during the print. This is much more accurate, however it emphasizes two incorrectnesses of the spiralize mode:

  • Flow on bottom and top layer are not correct i.r.t actual layer thickness
  • Last layer ends at approximaterly 1/2 layer height over the actual model

Those behaviors were similar in Cura 4.13.1, so I assume they can be safely ignored, and restoring the previous behavior is good enough for now.

CURA-12369

CURA-12369
The actual Z of the tapering spiral ends up at layer_thickness / 2.0 because for this layer, we print the last extrusion path twice with different Z offset and speeds, so at the end of the first path extrusion, the spiral should be at layer_thickness / 2.0 and that is where we want to print the tapering spiral.
@github-actions
Copy link
Contributor

github-actions bot commented Mar 3, 2025

Test Results

27 tests  ±0   27 ✅ ±0   5s ⏱️ ±0s
 1 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 6ad3c80. ± Comparison against base commit 7465edd.

♻️ This comment has been updated with latest results.

Copy link
Member

@rburema rburema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, but the component test shows this for me for the new code:

layer_under_top

@wawanbreton
Copy link
Contributor Author

Code looks good, but the component test shows this for me for the new code:

Oh, that is weird, totally not what I see 🤔 can you send the 3MF ?

@rburema
Copy link
Member

rburema commented Mar 4, 2025

Code looks good, but the component test shows this for me for the new code:

Oh, that is weird, totally not what I see 🤔 can you send the 3MF ?

It's just the project attached to the ticket 😅

rburema and others added 2 commits March 5, 2025 14:19
A path of constant height is added to 'finish' the top of spiralization -- this wasn't done at the correct height (at the very least in the preview for Windows but not in Linux -- which is still not completely clear why).

part of CURA-12369
@wawanbreton
Copy link
Contributor Author

The fix for Windows does break the behavior on Linux 😞

image

image

The tapering spiral now shows a jump up and ends 1.5 layer-height over the model. We will have to dig and see what makes this behavior inconsistent.

@rburema
Copy link
Member

rburema commented Mar 11, 2025

Hm, Windows also shows a similar jump, but smaller:

G1 X115.664 Y123.673 Z10.105 E183.92742
G1 X112.889 Y124.515 Z10.108 E184.02388
G1 X110 Y124.799 Z10.2 E184.11565
G1 X107.111 Y124.515 E184.20254
G1 X104.336 Y123.673 E184.28935

I copied this, and you screenshotted it; I'm not sure where the difference between 10.3 and 10.2 comes from.

Anyway, it's wrong here too -- still 1 layer above the model, rather than 1.5

rburema and others added 2 commits March 13, 2025 09:44
…non-spiralize' code.

Eventually, we should probably refactor this to use the scarf-seam code.

part of CURA-12369
@wawanbreton
Copy link
Contributor Author

Code looks good, now I'm gonna test it on Linux 🥁

@HellAholic HellAholic changed the base branch from 5.10 to main May 8, 2025 09:41
@jellespijker jellespijker requested a review from Copilot May 9, 2025 05:26
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR restores the previous spiralize wipe move behavior and improves the display of the spiral by more accurately showing the upward movement during printing. Key changes include an updated Point3LL constructor to accept a z-coordinate, modifications to the sendLineTo function signature and its corresponding calls, and refactored spiral path handling in LayerPlan.cpp using a lambda function.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/geometry/Point3LL.cpp Updated Point3LL constructor to include explicit z initialization
include/geometry/Point3LL.h Updated constructor declaration for Point3LL with an optional z
src/LayerPlan.cpp Modified sendLineTo signature and adjusted calls; added lambda for spiral paths
include/LayerPlan.h Updated sendLineTo declaration with an optional line_thickness parameter
Comments suppressed due to low confidence (2)

src/LayerPlan.cpp:3085

  • [nitpick] Consider renaming the parameter 'end_layer' to something like 'is_end_layer' for improved clarity of its boolean nature.
const auto writeSpiralPath = [&](const GCodePath& spiral_path, const bool end_layer) -> void

src/LayerPlan.cpp:3103

  • Constructing a new Point3LL with only p1.x_, p1.y_ and computed z_offset may omit the original z value of the spiral path. Verify that this intentional change correctly represents the desired position.
sendLineTo(spiral_path, Point3LL(p1.x_, p1.y_, z_offset), extrude_speed, layer_thickness_);

@HellAholic HellAholic merged commit ae38e60 into main May 9, 2025
24 checks passed
@HellAholic HellAholic deleted the CURA-12369_extra-wipe-move-with-spiralize branch May 9, 2025 09:36
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.

4 participants