Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

'New Tree' Refactors #7

Merged

Conversation

rburema
Copy link

@rburema rburema commented Dec 30, 2022

Propose to keep the code bases closer together, so it will be easier to merge (new) fixes.

Piezoid and others added 24 commits October 5, 2022 17:45
meshfix_maximum_extrusion_area_deviation is already in millimeters.
using get<coord_t> divides the value by 1000, instead it should be
get<size_t> or get<int>.
Simplify: fix scaling of maximum area deviation setting
…t_2"

This reverts commit e535e04, reversing
changes made to 034fe14.
Revert before merging to main

Contributes to CURA-9879
Revert before merging to main

Contributes to CURA-9879
Mac is being Mac again, also the
range-v3 library is used consistently
throughout the rest of the code

Contributes to CURA-9879
Put code in our code style -- including moving comments around, for better readability so definitely not all that is said there is mine ;-)

part of CURA-9879
Put code in our code style -- including moving comments around, for better readability so definitely not all that is said there is mine ;-) -- Includes workaround for Mac compiler bug stumbled into in last commit.

part of CURA-9879
Pass sby reference was erronously introduced by refactoring.

part of CURA-9879
…ish.

Manually cherry picked for CURA-9879 (git gave a bit of a mess when git cherry-pick was run...)
Will not do that much, but standard in the rest of the code.

part of CURA-9879
Added a note as well. I have no idea why the (in context) equivalent (even when the static_cast<size_t> is replaced by a simple cast to int) statement, using iota, doesn't work, but this does. It may just expose another bug, but best not to take the risk right now.

part of CURA-9879
It (moving the area-calculation to outside of the loop) _shoulnd't_ have caused issues (indicating that there's probably some deadlock possibility in the code as-is), but it _did_, so undo this part (for now).

part of CURA-9879
@ThomasRahm
Copy link
Owner

This would also undo "take disallowed areas into account when printing tree support". Is this intentional or should i cherry pick this commit?

// smooth upwards
for (LayerIndex layer_idx = 0; layer_idx < LayerIndex(layer_tree_polygons.size()) - 1; layer_idx++)
// Smooth upward.
for (const auto layer_idx : ranges::views::iota(0UL, layer_tree_polygons.size() - 1UL))
Copy link
Owner

Choose a reason for hiding this comment

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

I do not think it is very relevant as i don't think that this code can even be reached in the relevant edge-case, but theoretically if layer_tree_polygons.size() == 0 then this would underflow, while it would not underflow in my version as LayerIndex is a signed type.

@ThomasRahm
Copy link
Owner

Why is PropertyAreas and PropertyAreasUnordered in TreeSupportElements.h and not inside TreeSupport.h?
There may be a good reason why, it's just not where i would have been looking for it.

@rburema
Copy link
Author

rburema commented Jan 24, 2023

Hi @ThomasRahm . Sorry it took a bit longer to get back.

  • It's not intentional to undo that commit. I've cherry-picked those necessary to this branch.
  • I just fixed the 'empty lists' exception case with a little bit of defensive programming.
  • PropertyAreas and PropertyAreas where indeed only used in TreeSupport.___, so I moved those over.

I should also really mention that our team has decided that 5.3 is already very 'full' of features, and we've got bad experiences with putting everything in at once. As such, we'd like to take some more time on the New Tree Support as well. So it'll be in 5.4. (We try to release every two to three months.) In the mean-time, the alpha is still out, so people can continue with that for a while.

@ThomasRahm ThomasRahm merged commit 9675cde into ThomasRahm:tree_support_2 Jan 25, 2023
@rburema rburema deleted the CURA-9879_a_tree_by_xmas branch June 26, 2024 09:54
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.

5 participants