[CURA-13291] 'Sharpen' (wall) bridging conditions. - #2368
Open
Remco Burema (rburema) wants to merge 22 commits into
Open
[CURA-13291] 'Sharpen' (wall) bridging conditions.#2368Remco Burema (rburema) wants to merge 22 commits into
Remco Burema (rburema) wants to merge 22 commits into
Conversation
Use overhang angle as a threshold. As agreed upon with Print Processes and Materials. part of CURA-13291
This isn't production-ready (or even tested! or at all elegant...) and the parameter isn't actually in yet (magic number currently) -- but this is a sketch of how I planned to introduce the bridge wall max deviation. It's meant to have mostly straight walls when bridging; non-straight walls don't bridge very well and should be printed with normal or overhang parameters instead. part of CURA-13291
Feeling cute, might keep this. (A setting wasn't directly what was asked for in the ticket, but I think it makes sense.) part of CURA-13291
…segments. Compiles, but doesn't work yet (debugging will need to be done) -- Also needs a lot of parameter documentation still done. part of CURA-13291
… that're skipped. For the first bridge, we don't have the (begin) anchor-length. We can use the 'from start of wall' member parameter, but that doesn't take into account that there might have been bridges _before_ the proper first one that where skipped, because they didn't meet the (begin) anchoring requirements themselves. The lenghts of the skipped bridges shouldn't count towards the anchoring of the actual first bridge, so we skip those. During my debugging sessions while this code was WIP, I changed the - sign to a + sign because I forgot how this worked (even though I wrote it myself the week before) and thought it was a mistake. So, have an extended explanation here so people after me won't mess up either. part of CURA-13291
Contributor
Test Results31 tests 31 ✅ 5s ⏱️ Results for commit 5d3bf99. ♻️ This comment has been updated with latest results. |
Erwan MATHIEU (wawanbreton)
left a comment
Erwan MATHIEU (wawanbreton)
left a comment
Contributor
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'C++ Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 4393d47 | Previous: f1b43ee | Ratio |
|---|---|---|---|
SimplifyTestFixture/simplify_slot_noplugin |
19.683666672433393 ns/iter |
3.7417245469758185 ns/iter |
5.26 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: Jelle Spijker (@jellespijker) Erwan MATHIEU (@wawanbreton) Casper Lamboo (@casperlamboo) HellAholic
Erwan MATHIEU (wawanbreton)
left a comment
Erwan MATHIEU (wawanbreton)
left a comment
Contributor
There was a problem hiding this comment.
The code looks much better than the previous one, the structure makes a lot of sense and I can definitely understand it more easily ❤️
I have some small remarks about the syntax and possible improvements, but nothing big.
part of CURA-13291 Co-authored-by: Erwan MATHIEU <wawanbreton@gmail.com>
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
2 participants
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.
(For the newly introduced setting(s), see front-end PR: Ultimaker/Cura#21782 )
What was originally requested as part of the ticket:
Update the default behaviour of (wall+) bridging with the following changes:
+) From the context, I've taken to mostly mean the wall bridging specifically. I mostly didn't touch skin bridging, except for the changes to the bridging mask I suppose (but even that should mostly effect the wall-bridging rather than the skin-bridging, given that the changes will be mostly smaller than a wall-width).
++) Though this set of PR's includes a setting to toggle this behaviour, so the maintainers of 3rd party printers (and our own experiments) have a chance to --at least when it comes to whether or not to regard support as anchoring for bridges specifically-- the 'old' behaviour.
So, what was a relatively simple bundle of requests from our internal stakeholders, turned into a decent into unravelling sanity (maybe that's overstating it a little bit...), after I determined that a) even if only focussing on the walls, there where about 3-ish sources of truth where bridges where supposed to start/end, which b) made, in combination with none of these being complete, leading to (apparent) bugs, made it (almost?) completely impossible to even test the changes I'd made.
So halfway through I decided that enough was enough, the technical debt was out of hand, and re-implemented a large part of the wall-bridging logic. (This does mean that some of the initial commits here are overwritten to a large extent by later ones.)