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

feat(modeling): remove extrudeRectangular #1273

Merged
merged 1 commit into from
Sep 7, 2023

Conversation

platypii
Copy link
Contributor

This might be controversial. Let's use this PR for discussion on removing extrudeRectangular from V3.

The extrudeRectangular function is supposed to:

Extrude the given geometry by following the outline(s) with a rectangle.

But it's not doing that. If you "follow the outline(s) with a rectangle" you would get something more like the red shape, but extrudeRectangular returns the shape on the right:

extrude-rect

In reality extrudeRectangular is basically just doing:

extrudeLinear({}, offset({}, geometry))

This is not easy to fix, so I suggest that we simply delete extrudeRectangular. I would be surprised if anyone is using it, but if they are, they can use offset+extrude instead.

I think what was really intended here was a stroke function like the suggestion in #1272

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Does your submission pass tests?

@platypii platypii requested review from z3dev and hrgdavor July 19, 2023 02:28
@z3dev
Copy link
Member

z3dev commented Jul 23, 2023

@platypii interesting. i really didn't like this extrude function from day one as, as you mentioned, is just putting two functions together. users should be able to figure out how to do this through a simple example.

i also don't like 'torus' either, as that is simply extrude rotate with a sphere. it makes no sense to implement primitives via complex functionality. primitives should exist by themselves. examples should show how to create a torus.

@platypii platypii force-pushed the V3-remove-extrudeRectangular branch from 0102b68 to b3de6c4 Compare July 24, 2023 20:41
@platypii
Copy link
Contributor Author

Rebased to fix merge conflicts.

Can anyone tell me what exactly is "rectangular" about extrudeRectangular?

@z3dev
Copy link
Member

z3dev commented Jul 24, 2023

Rebased to fix merge conflicts.

Can anyone tell me what exactly is "rectangular" about extrudeRectangular?

Nope. You could go back and look at V1, which may have had the same issues, or different issues.

V2 allows 'corners' to be passed into extrudeRectangular() as well. So there are a few more options.

@z3dev
Copy link
Member

z3dev commented Sep 2, 2023

From CSG.js V1... math/Path2.js

// Expand the path to a CAG
// This traces the path with a circle with radius pathradius
expandToCAG: function (pathradius, resolution) {
}

@platypii
Copy link
Contributor Author

platypii commented Sep 6, 2023

Earliest I could find was this commit from 2012: joostn/OpenJsCad@b059ed95

So... can we kill it? 🔥

@z3dev z3dev merged commit 6c0cb4a into jscad:V3 Sep 7, 2023
2 checks passed
@platypii platypii deleted the V3-remove-extrudeRectangular branch September 7, 2023 01:14
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.

2 participants