cbernardo / libIGES

Implementation of the IGESv5.3 specification
http://cbernardo.github.io/libIGES
GNU Lesser General Public License v2.1
58 stars 18 forks source link

Completes Start and End Point Identification for Type 126 Entities #20

Closed justingravett closed 4 years ago

justingravett commented 4 years ago

If "USE_SISL" is false, IGES_ENTITY_126::GetStartPoint and IGES_ENTITY_126::GetEndPoint do not identify the start and end points of the NURBS curve. "pt" remains set to its initialization value unless a transformation matrix is defined for the NURBS curve. When calling IGES_ENTITY_102::AddSegment, any segment after the first two fail to be added because IGES_ENTITY_102::IsClosed incorrectly returns true.

justingravett commented 4 years ago

Thank you for reviewing the PR. Regarding (a), a requirement of 3 coefficients is checked with the following at the beginning of each function:

if( nCoeffs < 2 ) return false;

However, I misunderstood what "nCoeffs" represents, assuming it was equivalent to the size of "coeffs". I just pushed up a fix for that, which includes considerations for rational curves.

If the curve is rational, is my understanding correct that their representation is (x,y,z,w) and not (xw,yw,z*w,w)? For the second case, each control point would have to be divided by its associated weight, which I am currently not doing.

cbernardo commented 4 years ago

Thanks for fixing this; the problem was reported before but I could not reproduce it because I did not know that SISL was not being linked. I wouldn't be surprised if there are other bugs lurking where SISL is not defined. In the case of rational splines there is only a single weight for the control point so you are correct, it is (x, y, z, w).

justingravett commented 4 years ago

No problem, and thanks for reviewing and integrating it. However, should the start point indexes be zero instead of one based? If I’m understanding correctly, coeffs[1] -> y_0, coeffs[2] -> z_0, & coeffs[3] -> x_1.

I’m pretty sure this fix also will close Issue #15 (https://github.com/cbernardo/libIGES/issues/15). I remember experiencing the problem myself when I recreated the tutorial a while back. One or two compound curves were incomplete, so the object would not trim correctly.

I’ll keep an eye out for any other cases where there is no SISL alternative. I also need to figure out why it’s not being linked correctly in both my LibIGES build and OpenVSP.

Thanks,

Justin

From: cbernardo notifications@github.com Sent: Friday, March 20, 2020 12:00 AM To: cbernardo/libIGES libIGES@noreply.github.com Cc: Justin Gravett justin.gravett@esaero.com; Author author@noreply.github.com Subject: Re: [cbernardo/libIGES] Completes Start and End Point Identification for Type 126 Entities (#20)

Thanks for fixing this; the problem was reported before but I could not reproduce it because I did not know that SISL was not being linked. I wouldn't be surprised if there are other bugs lurking where SISL is not defined. In the case of rational splines there is only a single weight for the control point so you are correct, it is (x, y, z, w).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/cbernardo/libIGES/pull/20#issuecomment-601560645, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AE6WNIRCNSS66MS7TGXMST3RIMH6HANCNFSM4LO5FOFA.

cbernardo commented 4 years ago

You're right; sorry about the index bug on the start point.