-
Notifications
You must be signed in to change notification settings - Fork 74
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
add support for flipped extrude #940
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I was just curious about what the expected behavior should be if there are voids defined. I tested it out just to see and this is what it looks like with the current code:
I think this is preferred over the inverse for voids, but just wanted to point it out!
Code if you want to copy/paste for yourself:
var profileNormal = new Profile(Polygon.Rectangle(1, 1), Polygon.Rectangle(0.5, 0.5));
var normalExtrude = new Extrude(profileNormal, 1, Vector3.ZAxis);
var profileFlipped = new Profile(Polygon.Rectangle(1, 1).TransformedPolygon(new Transform(2, 0, 0)), Polygon.Rectangle(0.5, 0.5).TransformedPolygon(new Transform(2, 0, 0)));
var flippedExtrude = new Extrude(profileFlipped, 1, Vector3.ZAxis, false, true);
var geoNormal = new GeometricElement
{
Representation = new Representation(normalExtrude)
};
var geoFlipped = new GeometricElement
{
Representation = new Representation(flippedExtrude)
};
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @andrewheumann)
Elements/src/Geometry/Solids/Extrude.cs
line 84 at r1 (raw file):
/// <param name="height"></param> /// <param name="direction"></param> /// <param name="isVoid"></param>
Add /// <param name="flipped"></param>
for documentation... Also do we not need to call it @flipped
? I don't know the exact reasoning for the @
symbol in all these cases across the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I would expect for an "inside out extrude with voids." Looks a little funny, but I think the alternative would too!
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @anthonie-kramer)
Elements/src/Geometry/Solids/Extrude.cs
line 84 at r1 (raw file):
Previously, anthonie-kramer (Anthonie Kramer) wrote…
Add
/// <param name="flipped"></param>
for documentation... Also do we not need to call it@flipped
? I don't know the exact reasoning for the@
symbol in all these cases across the code.
The @sign is a legacy from when we codegenned ALL our classes, even the ones built in to elements. The @ symbol MOSTLY does nothing, but it helps protect us if a variable name from the JSON is a reserved word in C#, like class
. If you did Polygon class
, you'd get a compiler error, but Polygon @class
builds. Still, it really doesn't serve us anymore — I've gotten rid of the others here, and added xml comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @andrewheumann)
Elements/src/Geometry/Solids/Extrude.cs
line 84 at r1 (raw file):
Previously, andrewheumann wrote…
The @sign is a legacy from when we codegenned ALL our classes, even the ones built in to elements. The @ symbol MOSTLY does nothing, but it helps protect us if a variable name from the JSON is a reserved word in C#, like
class
. If you didPolygon class
, you'd get a compiler error, butPolygon @class
builds. Still, it really doesn't serve us anymore — I've gotten rid of the others here, and added xml comments.
Sounds good! Thanks for the context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 1 approvals obtained
BACKGROUND:
ConstructedSolid
direct from faces, sinceExtrude
has a built-in check to ensure that normals point out. This works fine, but means that we serialize much more data than is needed in the model: we have to store every vertex, edge, and face, which makes the model json heavy.DESCRIPTION:
Flipped
property, which inverts the normal check when building the extrude. ifFlipped
is true, the profile is flipped w/r/t the extrusion direction, resulting in an inside-out shape.TESTING:
Flipped
parameter has the desired effect.FUTURE WORK:
REQUIRED:
CHANGELOG.md
.This change is