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

Regression fix for Elements.Geometry.Solids.Sweep #1016

Merged
merged 7 commits into from
Oct 5, 2023

Conversation

DmytroMuravskyi
Copy link
Contributor

@DmytroMuravskyi DmytroMuravskyi commented Sep 1, 2023

BACKGROUND:

  • Where was reported an issue about Sweep: Regression Elements.Geometry.Solids.Sweep #1007
  • The reason code worked before is that setbacks parameters were just ignored.
  • Sweep in the issue crashes because setback ends exactly on second vertex.
  • Setbacks need extra handling for special cases as it always assumes that start setback is on first segment, end - on last. This is not the case as they can easily exceed.

DESCRIPTION:

  • Changed GetSubdivisionParameters to use startSetbackDistance and endSetbackDistance since they were just ignored.
  • Changed Polyline.Frames and Polygon.Frames so they return only frames between setbacks. Since CreateMiterTransform and CreateOrthogonalTransform are focused on vertices, Transforms on setbacks are created manually.

TESTING:

  • Added tests for updated functions as well as regression Sweep.

FUTURE WORK:

  • Polyline.TransformAt feels incorrect as it says that u is "between 0.0 and length" but it's treated as 0 to 1 value. Also, since calculation inside is length based, including normal calculation, curve length can possibly influence frame vectors in a wrong way.

REQUIRED:

  • All changes are up to date in CHANGELOG.md.

COMMENTS:

  • I will be away for a while, so feel free to steal this code if necessery.

This change is Reviewable

@ikeough ikeough added this to the 2.1 milestone Sep 6, 2023
Copy link
Member

@andrewheumann andrewheumann left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 9 files at r1, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @DmytroMuravskyi)


Elements/src/Geometry/Polygon.cs line 2094 at r1 (raw file):

            // Calculate number of frames. 2 frames corresponding to end parameters.
            // 1 if startIndex == endIndex.
            var lenght = endIndex - startIndex + 3;

lenght => length


Elements/src/Geometry/Polyline.cs line 306 at r1 (raw file):

            // Calculate number of frames. 2 frames corresponding to end parameters.
            // 1 if startIndex == endIndex.
            var lenght =  endIndex - startIndex + 3;

lenght => length


Elements/test/PolygonTests.cs line 2234 at r1 (raw file):

            Assert.True((Vector3.YAxis - Vector3.XAxis).IsParallelTo(frames[4].ZAxis));

            frames = polygon.Frames(2, 2);

polyhon.Frames(1, 1) will throw an exception here.

Copy link
Contributor Author

@DmytroMuravskyi DmytroMuravskyi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @andrewheumann)


Elements/src/Geometry/Polygon.cs line 2094 at r1 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

lenght => length

Done.


Elements/src/Geometry/Polyline.cs line 306 at r1 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

lenght => length

Done.


Elements/test/PolygonTests.cs line 2234 at r1 (raw file):

Previously, andrewheumann (Andrew Heumann) wrote…

polyhon.Frames(1, 1) will throw an exception here.

Good catch. Added index rounding for Polygon.

Copy link
Member

@andrewheumann andrewheumann left a comment

Choose a reason for hiding this comment

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

The frames being found along the curve are flipped compared to how they used to be — I think this is an unintended regression. This also results in sweeps having "inside-out" normals:

Elements 1.6.0:

image.png

Elements 2.0.0:

image copy 1.png

This PR:
image copy 2.png

All three samples are running this code:

var polygon = new Polyline((0, 0), (3, 0), (3, 3), (0, 3));
var model = new Model();
model.AddElement(new ModelCurve(polygon, BuiltInMaterials.XAxis));
var frames = polygon.Frames(0.1, 0.1);
model.AddElements(frames.SelectMany(f => f.ToModelCurves()));

var profile = Polygon.L(0.2, 0.2, 0.05);
var ge = new GeometricElement {
    Representation = new Sweep(profile, polygon, 0.1, 0.1, 0, false)
};
model.AddElement(ge);

We have been inconsistent about this in the past, but I think the convention in our code is that the z of a curve's frame is opposite the curve's tangent at that point. (Don't ask me why it is this way, I would 100% expect Z+ to be the same as the tangent.)

Reviewed 3 of 9 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained

Copy link
Contributor Author

@DmytroMuravskyi DmytroMuravskyi left a comment

Choose a reason for hiding this comment

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

Yes, I missed the fact that they are adjusted. Now frames look like this
image.png
Note that Frames behaves differently from TrasformAt as first and last frames are not rotated based on position if they are somewhere in the middle of segment but just . One exception is when setback is coincides with a corner, then Frame is also the same as for corner vertex - rotated. I also made tests more explicit about expected results.

Reviewable status: 1 change requests, 0 of 1 approvals obtained

@andrewheumann
Copy link
Member

Looks good to me, thanks!

Copy link
Member

@andrewheumann andrewheumann left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@andrewheumann andrewheumann merged commit 926ca5e into hypar-io:master Oct 5, 2023
1 check passed
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.

3 participants