Revert to use of cos^2 in LiftDrag plugin #2273
Merged
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
The changes in #2189 have caused a regression for our upstream users. This fix disables moment calculations by defaulting Cma to zero. It should help mitigate some of the regression. There is also an [ongoing discussion](#2189 (comment)) as to whether the cos term should be cos^2 or cos. Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
In response to #2189 (comment) As suggested by @srmainwaring I also renamed cosSweepAngle -> cos2SweepAngle to prevent further confusion. Signed-off-by: Arjo Chakravarty <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## gz-sim7 #2273 +/- ##
===========================================
- Coverage 64.74% 64.72% -0.03%
===========================================
Files 357 357
Lines 29141 29139 -2
===========================================
- Hits 18868 18860 -8
- Misses 10273 10279 +6 ☔ View full report in Codecov by Sentry. |
Base automatically changed from
arjo/feat/disable_moments_by_default
to
gz-sim7
December 27, 2023 07:17
|
Looks like it needs a rebase and a commit squash to the 3 lines changed from cos->cos2. |
It's OK theres no need to do anything since we squash and rebase before we merge into main in anycase. |
srmainwaring
suggested changes
Dec 30, 2023
Co-authored-by: Rhys Mainwaring <[email protected]> Signed-off-by: Arjo Chakravarty <[email protected]>
srmainwaring
approved these changes
Jan 2, 2024
azeey
reviewed
Jan 2, 2024
src/systems/lift_drag/LiftDrag.cc
Outdated
| @@ -327,8 +327,9 @@ void LiftDragPrivate::Update(EntityComponentManager &_ecm) | |||
| double sinSweepAngle = math::clamp( | |||
| spanwiseI.Dot(velI), minRatio, maxRatio); | |||
|
|
|||
| // get cos from trig identity | |||
| double cosSweepAngle = sqrt(1.0 - sinSweepAngle * sinSweepAngle); | |||
| // The sweep adjustment depends on the velocity component normal to the wing leading | |||
There was a problem hiding this comment.
linter seems unhappy about these lines.
| double cosSweepAngle = sqrt(1.0 - sinSweepAngle * sinSweepAngle); | ||
| // The sweep adjustment depends on the velocity component normal to the wing leading | ||
| // edge which appears quadratically in the dynamic pressure, so scale by cos^2 . | ||
| double cos2SweepAngle = 1.0 - sinSweepAngle * sinSweepAngle; |
There was a problem hiding this comment.
@frede791 will this break your models?
There was a problem hiding this comment.
No, shouldn't be an issue for the PX4 models.
Signed-off-by: Arjo Chakravarty <[email protected]>
bperseghetti
approved these changes
Jan 8, 2024
mjcarroll
approved these changes
Jan 8, 2024
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.
🦟 Bug fix
Fixes #
Summary
In response to #2189 (comment)
As suggested by @srmainwaring I also renamed cosSweepAngle ->
cos2SweepAngle to prevent further confusion.
Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-bymessages.