[CURA-12961] Spike to add auto-brim option; DRAFT!#2320
[CURA-12961] Spike to add auto-brim option; DRAFT!#2320
Conversation
Let's see if we can drag this potentially useful feature to Cura as well ;-) (rewrite from stuff in Orca; they've got more than enough of our source in their thing without any attribution more extensive than I did here and in the comment, so aparently it's fine :-p). So far, it seems to spit out figures within the realm of reasonability, though you have to make the model really long and thin for it to split out anything above the minimum threshold of 5mm. part of investigation spike CURA-12961
The normal one should work, even though it's way larger, since these are areas measured in square micron, and our own epsilon value is for muicron values. done as part of CURA-12961
wawanbreton
left a comment
wawanbreton
left a comment
There was a problem hiding this comment.
Code is a bit drafty as advertised, but overall looks good. Also you are slightly abusing std::accumulate 😛
| return all_brim_offsets; | ||
| } | ||
|
|
||
| void computeOverSegments(const Polygon& poly, const std::function<void(const Point2F&, const Point2F&)>& func) |
There was a problem hiding this comment.
Would it make sense that this function is a method of the Polygon class (or even Polyline) ?
Also maybe with a more explicit naming.
There was a problem hiding this comment.
Yes and yes -- I wanted to have it self-contained for the spike, but we can move anything that is accepted to polygon/-line.
Any proposal for the name?
| } | ||
| } | ||
|
|
||
| bool compShapeProperties(const Shape& polys, Point2F& total_centroid, Point2F& total_moment) |
There was a problem hiding this comment.
Maybe move to Shape or LinearAlg2D ?
| { | ||
| return std::max(value, static_cast<float>(mesh.settings_.get<coord_t>(setting_name.data()))); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Using accumulate in this case is quite confusing 🤔 could you use ranges::max_element instead, with a projection function ?
There was a problem hiding this comment.
Ah, I was trying to find that function 2 weeks ago. Yes, we should use that instead.
| AABB3D(), | ||
| [](const AABB3D& aabb, const Mesh& mesh) | ||
| { | ||
| return AABB3D(mesh.getAABB()).include(aabb); |
There was a problem hiding this comment.
As far as I can see, mesh.getAABB() return a AABB3D, so why the conversion ?
There was a problem hiding this comment.
I think that might just be a leftover from when I had written something else there instead.
| // Calculate the result. | ||
| constexpr float height_to_area_normalization = 1920.0f; | ||
| const float height_to_area | ||
| = std::max(max_height / second_moment.x_ * INT2MM(width_depth.y_), max_height / second_moment.y_ * INT2MM(width_depth.x_)) * max_height / height_to_area_normalization; |
There was a problem hiding this comment.
Can either second_moment.x_ or second_moment.y_ be null anyhow ?
There was a problem hiding this comment.
Hm, I think we should guard against that in any case. But that would be an object that'd require an infinitely large brim then 😄
I think I see another (small) division by zero potential in the accumulate_areas above as well now that you've pointed out this one.
Co-authored-by: Erwan MATHIEU <[email protected]>
2 participants
Very W.I.P. part of a spike to investigate whether we can actually automatically detect brim-width / basis for discussion.
Largely copied from other (open) sources (then heavily rewritten) at the moment.
To run from the front-end, use this PR: Ultimaker/Cura#21531