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

Implemented Fuselage Profile Type: Rectangle (Issue #909) #1005

Open
wants to merge 41 commits into
base: master
Choose a base branch
from

Conversation

merakulix
Copy link
Contributor

Description

Implementing the rectangle profile type as an element of the CPACS standard profile type covers one part of issue #909 on parametric profile types.
This approach is based on point lists (std::vector<gp_Pnt>) for approximation of curves, since the lofting algorithms don't support rational B-Splines yet.

Changes in tiglcommonfunctions.* :

Added new global functions:
- BuildWireRectangle(... , ): Builds the wire which is needed to build profiles. Uses these newly implemented functions:
- ApproximateArcOfCircleToRationalBSpline(... , ): Approximation to a rational B-Spline via point list was needed, since the lofting algorithm doesn't support rational B-Splines yet
- Linspace(... , ): Useful function when building point lists

Changes in CCPACSFuselageProfile.* :

Added new member function

  • BuildWiresRectangle(...,): - uses buildWireRactangle(..,) from tiglcommonfunctions.*

Changed member function:

  • BuildWires(...): Added conditional statement to choose right method for building the wire, depending on profile type

How Has This Been Tested?

Added Unit Tests:

  • testFuselageStandardProfileRectangle.cpp:
    • added testfiles that define a fuselage with mixed profiles:
      • simpletest_standard_profile_rectangle_circle_guides.cpacs.xml
      • simpletest_standard_profile_rectangle_circle_guides.cpacs.xml

a circle profile with kinks and a rectangle profile with and without corner radius, as shown in the image below

grafik
grafik

circle profile, rectangle profile with and without corner radius given, and guide curves, as shown in the images below

grafik
grafik

Checklist:

  • [x ] A test for the new functionality was added.
  • [x ] All tests run without failure.
  • [x ] The new code complies with the TiGL style guide.
  • New classes have been added to the Python interface.
  • API changes were documented properly in tigl.h.

Copy link
Contributor

@joergbrech joergbrech left a comment

Choose a reason for hiding this comment

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

Thanks a lot @merakulix! The images look very impressive! 😃 I have a few comments, see below.

src/common/tiglcommonfunctions.cpp Outdated Show resolved Hide resolved
src/common/tiglcommonfunctions.cpp Outdated Show resolved Hide resolved
src/common/tiglcommonfunctions.cpp Outdated Show resolved Hide resolved
src/common/tiglcommonfunctions.h Outdated Show resolved Hide resolved
src/common/tiglcommonfunctions.h Outdated Show resolved Hide resolved
src/geometry/CTiglPointsToBSplineInterpolation.cpp Outdated Show resolved Hide resolved
src/fuselage/CCPACSFuselageProfiles.cpp Outdated Show resolved Hide resolved
ASSERT_THROW(loft.Shape(), tigl::CTiglError);
}


Copy link
Contributor

Choose a reason for hiding this comment

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

All tests above this line test a function from commonfunctions.h. Let's move these tests to TiglCommonFunctions.cpp file. They don't need the fixture with the CPACS files at all.

Comment on lines 166 to 167
tigl::dumpShape(fuselage->Shape(), "mydir", "fuselageMix");
tigl::dumpShape(wing->Shape(), "mydir", "wing1");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tigl::dumpShape(fuselage->Shape(), "mydir", "fuselageMix");
tigl::dumpShape(wing->Shape(), "mydir", "wing1");

Let's not dump shapes as part of unit tests.

tests/unittests/testFuselageStandardProfileRectangle.cpp Outdated Show resolved Hide resolved

if (!(cornerRadius == 0.0)){
//calculate the number of points required to maintain the minimum distance (<tol) between the bisector of two neighboring points on an arc circle
nb_points = (int) (M_PI/(acos(1-tol/cornerRadius)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Casting to int is rounding down to the next lower integer, which means we have a slightly smaller resolution than required by the tolerance. We should round up here.

Comment on lines +1259 to +1261
if (curve->Degree()<2){
curve->IncreaseDegree(2);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't fully understand why this fixes the issue with the guide curves, but fine, let's keep this as a workaround for now. But then please add a code comment description of why these three lines were added. Please be so specific, that a future reader can replicate the problem

Copy link

codecov bot commented Jun 3, 2024

Codecov Report

Attention: Patch coverage is 92.54658% with 12 lines in your changes missing coverage. Please review.

Project coverage is 69.08%. Comparing base (42935bb) to head (67be429).
Report is 4 commits behind head on master.

Current head 67be429 differs from pull request most recent head 49bc34e

Please upload reports for the commit 49bc34e to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1005      +/-   ##
==========================================
+ Coverage   68.97%   69.08%   +0.11%     
==========================================
  Files         299      299              
  Lines       26499    26608     +109     
==========================================
+ Hits        18277    18383     +106     
- Misses       8222     8225       +3     
Files Coverage Δ
src/common/tiglcommonfunctions.h 100.00% <ø> (ø)
src/common/tiglcommonfunctions.cpp 76.18% <97.40%> (+1.59%) ⬆️
src/geometry/CTiglPointsToBSplineInterpolation.cpp 87.40% <50.00%> (-2.36%) ⬇️
src/fuselage/CCPACSFuselageProfile.cpp 82.27% <92.10%> (+1.68%) ⬆️

... and 2 files with indirect coverage changes

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.

None yet

3 participants