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

Pin CP to axis (Fix 654) #657

Merged
merged 2 commits into from
May 8, 2020
Merged

Conversation

JoePfeiffer
Copy link
Contributor

My test for three or more fins in BarrowmanCalculator.calculatedNonAxialForces() turns out to be the only place in the code where it's possible to set the CP off-axis. Everywhere else, CP is pinned to the axis; the code would be clearer if CP were just a Double instead of a Coordinate (but that's a bigger change than I want to make right now).

So anyway, this pins CP to the axis in this one case as well by removing the "if" statement. In essence, the code now assumes radial symmetry; even if we're only looking at a single fin we assume there is a matching fin somewhere.

We may want to investigate whether the CP should be allowed to be off-center (say, modeling a glider) in the future, but not right now.

…ialForces()

It turns out that the assumption of radial symmetry is absolutely pervasive in our CP calculations.  Looking at FinSetCalc.calculateNonAxialForces(), we don't actually compute the CP of a fin -- we calculate its X value, and pin it to Y=Z=0.

The only exception to this is in BarrowmanCalculator.calculateNonAxialForces(), where the pinning only takes place if there are three or more fins in a FinSet.  If we remove the test for number of fins in the FinSet, we are in essence making the assumption that there are more fins *somewhere* which will end up being radially symmetric.
@TravisBuddy
Copy link

Hey @JoePfeiffer,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 5f8901a0-9071-11ea-931a-79bfa81bc315

@teyrana
Copy link
Member

teyrana commented May 7, 2020

Okay, this is the opposite of #655. It's silly to do both. We should either pin both the calculation AND the display, or neither.

And fwiw, I'd rather calculate it in 3d. If a rocket has an off center Cp, that's REALLY important -- that's an unsafe rocket, and a crash-on-first-flight.

Even for gliders, I would argue. Wouldn't hurt to get some feedback from gliders designers, ofc

@JoePfeiffer
Copy link
Contributor Author

JoePfeiffer commented May 7, 2020

I believe this is behavior that goes back a long, long time -- where can I find the source for 15.03? Was that the initial checkin to the old Master branch? If 15.03 calculates it off-center this is a regression we need to look at now (and fixing will be painful); if it has been the behavior all along investigating it can wait until after next release. I sort of like the idea of having the renderings display it wherever we calculate it and (assuming this isn't a regression) pinning it in the calculation. That way if we do end up calculating off-center when appropriate in the future we won't get bitten by the rendering not showing it.

FWIW, I created the attached rocket in 15.03 with two fins at a 120 degree angle; the component analysis only shows the CP x location (if it's calculating it off-center, the only place I can see to get that information is in the renderings, which may well be pinning it in the display).
two-fins.ork.zip

Just marked this PR as a draft until we figure out whether it's a regression.

@JoePfeiffer JoePfeiffer marked this pull request as draft May 7, 2020 15:30
@teyrana
Copy link
Member

teyrana commented May 7, 2020

I believe this is behavior that goes back a long, long time -- where can I find the source for 15.03? Was that the initial checkin to the old Master branch?

It's just a tag

If 15.03 calculates it off-center this is a regression . . .

I don't quite follow here... do you mean if 15.03 calculates a non-zero CP on the example rocket in #654 #648 ?

I'm pretty sure (and we'll check this) that 15.03 is just too simple in the way it calculates Cp -- iirc, it only calculates in the x-y plane, and simply ignores the x-z plane)

Personally, I consider this behavior to be somewhere between a barely-tolerable-simplification, and an outright bug (in 15.03); but this question has been lurking behind a bunch of the Cp issues & PRs for the past year. We'll can't really sweep it under the rug any longer, and this is as good a reason to surface it, as any.

I sort of like the idea of having the renderings display it wherever we calculate it and (assuming this isn't a regression) pinning it in the calculation. That way if we do end up calculating off-center when appropriate in the future we won't get bitten by the rendering not showing it.

/agreed

FWIW, I created the attached rocket in 15.03 with two fins at a 120 degree angle; the component analysis only shows the CP x location (if it's calculating it off-center, the only place I can see to get that information is in the renderings, which may well be pinning it in the display).
two-fins.ork.zip

Yea, so that's the other thing -- if we're pinning x = y = 0 in the 2d render, we should pin them in the 3d render, too. Or the converse.

Just marked this PR as a draft until we figure out whether it's a regression.

Good point. I've converted #655, as well.

@JoePfeiffer
Copy link
Contributor Author

If 15.03 calculates it off-center this is a regression . . .

I don't quite follow here... do you mean if 15.03 calculates a non-zero CP on the example rocket in #654 #648 ?
I meant if the code in 15.03 could calculate an off-center CP, whether we have an example at present that shows it or not.

I'm pretty sure (and we'll check this) that 15.03 is just too simple in the way it calculates Cp -- iirc, it only calculates in the x-y plane, and simply ignores the x-z plane)

Personally, I consider this behavior to be somewhere between a barely-tolerable-simplification, and an outright bug (in 15.03); but this question has been lurking behind a bunch of the Cp issues & PRs for the past year. We'll can't really sweep it under the rug any longer, and this is as good a reason to surface it, as any.

After a look at the FinSet CP calculations, I'm confident 15.03 can't generate an off-axis CP. My view is that we've been at this release long enough that anything that isn't a a regression or new bug is acceptable; more accurate calculation of the CP (in particular, whether it should be off-axis in some cases) should come after the next release.

The issue that's been behind the CP and rendering issues is that I introduced a single circumstance in which an off-center CP can be calculated, without making that possibility be the general case (in which case all the off-center CPs ought to cancel out in the various fins-on-pods examples we've been fighting with).

@teyrana
Copy link
Member

teyrana commented May 7, 2020

fyi, the code that calculates CP in 15.03 is here for fins ... (for example).

And you can see from Line 125 That it only calculates the CP in the x-y plane.

And you can see from line 232 and especially 260 that it's hard coded to only calculate the x-component.

@teyrana
Copy link
Member

teyrana commented May 7, 2020

So you're saying that if we just merge this (657) and 655, we'll be done with the off-axis CP values?

@JoePfeiffer
Copy link
Contributor Author

So you're saying that if we just merge this (657) and 655, we'll be done with the off-axis CP values?

I believe so, yes.

@JoePfeiffer JoePfeiffer marked this pull request as ready for review May 8, 2020 00:34
@teyrana teyrana merged commit 90173c3 into openrocket:unstable May 8, 2020
@JoePfeiffer JoePfeiffer deleted the fix-654 branch May 14, 2020 21:10
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