-
Notifications
You must be signed in to change notification settings - Fork 68
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
Tanks #1058
Open
joergbrech
wants to merge
79
commits into
master
Choose a base branch
from
tanks
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Tanks #1058
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…; add configuration
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1058 +/- ##
==========================================
- Coverage 69.50% 69.47% -0.03%
==========================================
Files 301 307 +6
Lines 26912 27510 +598
==========================================
+ Hits 18705 19113 +408
- Misses 8207 8397 +190
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Thanks @MarAlder! Just a heads up: This is a big contribution and I am going to need a bit for the review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR adds
fuelTanks
. These tanks consist of several vessels. Each vessel can be made up of several skin layers to represent composite materials. There is usually a vacuum between the vessels. A vessel is constructed in the same way as a fuselage - in principle like a matryoshka of fuselages.Note: It is still an open CPACS discussion how to realize varying skin thicknesses of a vessel. This would also affect the
fuselage
and will therefore be a separate issue.What is new compared to
fuselage
or similar elements withparentUID
: The tank vessels are direct child elements of thefuelTank
element, and thus not referencing to this viaparentUID
. Furthermore, they should not only follow thetranslation
of thefuelTank
element, but also thescaling
androtation
. Therefore, additional constructors are added toCTiglRelativelyPositionedComponent
, along with methods to apply scaling and rotation to the transformation matrix.The added code follows the proposed clang settings.
How Has This Been Tested?
Thorough testing has been implemented for C++ and Python code:
simpletest-fuelTanks.cpacs.xml
tiglTanks.cpp
test_fuelTanks.py
The tests cover all elements of the fuelTanks added to CPACS. Perhaps the Python tests in particular go a bit beyond what would only concern the tanks, as they also test the correct export of the generated classes. If automated Python tests for the generated classes are part of future TiGL releases, this could be removed. For now, I feel a bit more comfortable as it is not always clear to me how exactly the
configuration.i
is built.Screenshots, that help to understand the changes(if applicable):
Checklist:
EDIT: This PR is just a replicate from #1057, because our CI has problems with PRs from forked repos, see #1059. Thanks @MarAlder for all the work!