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

Fixes ALS017 (IVS-282) #328

Merged
merged 5 commits into from
Feb 1, 2025
Merged

Fixes ALS017 (IVS-282) #328

merged 5 commits into from
Feb 1, 2025

Conversation

civilx64
Copy link
Contributor

@civilx64 civilx64 commented Dec 9, 2024

  • newer version of ifcopenshell corrects issue with parabolic vertical curves in imperial units
  • change in step implementation now uses the correct values for u in evaluate(u) for calculating placement transforms at the ends of alignment segments.

…ment representations

The length of each segment corresponds to the total length along the parent curve.
However, for continuity calculations, we need to provide the total length along the *base* curve.

(IVS-282)
@civilx64 civilx64 requested a review from Ghesselink December 9, 2024 11:04
@civilx64 civilx64 changed the title Fixes ALS017 Fixes ALS017 (IVS-282) Dec 9, 2024
Comment on lines 206 to 207
u = abs(self.segment_to_analyze.Placement.Location.Coordinates[0] - \
self.previous_segment.Placement.Location.Coordinates[0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this a bit cyclical? We want to assess basically whether segment to analyse and previous segment align, but if we do that based on assessing the respective placements we would miss some cases where the length of previous is actually not enough to reach the placement of current?

In other words: this assesses continuity of vertical in the Y axis, but doesn't account for gaps in the domain of the vertical profile, Is that a problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Potentially. I had gotten wrapped around the axle with confusing length along the base curve versus segment length of the parent curve. Using the placements was the quickest way to resolve this as well as confirm my understanding of the piecewise function implementation.

The cleanest way forward IMHO is to add a property endPoint with type taxonomy::cast<taxonomy::matrix4d> to curve_segment_evaluator given that we have both length_ and projected_length_ available. Then (I think) we can just call pwf.end_point() from python to get the calculated point at the end of the segment - versus dealing with the complication of length versus projected_length on the python side.

I'm keen to take this on as it will confirm my burgeoning C++ capabilities 😉.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IfcOpenShell PR#6006 now provides correct values of function_item.length() for vertical alignment (IfcGradientCurve) segments. Our calling code was adjusted accordingly.

Updated to current geometry evaluation methods for alignment segments.
Adjusted IFC test files to reflect corrections to direction vectors and parabolic segment lengths.
 Updated CI pipeline to use the latest IfcOpenShell build.

 (IVS-282)
@civilx64
Copy link
Contributor Author

I also made some adjustments / corrections to the unit test files:

  1. Normalized the vector components of IfcDirection. (Technically they do not have to be normalized but in practice they usually are).
  2. Corrected the values of SegmentLength for parabolic vertical curve segments. SegmentLength is along the parabola, and not just the difference in x between PVT and PVC.

@civilx64 civilx64 requested a review from aothms January 26, 2025 06:06
Copy link
Collaborator

@aothms aothms left a comment

Choose a reason for hiding this comment

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

Nice work for taking it all the way to C++ to track down the issues here!

@aothms aothms merged commit f69dcea into development Feb 1, 2025
2 checks passed
@aothms aothms deleted the fix/IVS-282-ALS017 branch February 1, 2025 17:25
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