Add support for setting ic/vw-mag-fps

[seanmcleod]

Based on the issue reported in discussion - #1088

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 24.91%. Comparing base (56c6662) to head (56aa719).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1093   +/-   ##
=======================================
  Coverage   24.91%   24.91%           
=======================================
  Files         168      168           
  Lines       18104    18105    +1     
=======================================
+ Hits         4510     4511    +1     
  Misses      13594    13594           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bcoconni]

A couple of minor updates could be made and it would be good if the unit test could be updated to test the new setter:

jsbsim/tests/unit_tests/FGInitialConditionTest.h

Lines 347 to 372 in 56c6662

	void testWindVelocity() {
	FGFDMExec fdmex;
	FGInitialCondition ic(&fdmex);
	ic.SetWindDownKtsIC(1.0);
	TS_ASSERT_DELTA(ic.GetWindDFpsIC(), ktstofps, epsilon);
	ic.SetWindNEDFpsIC(1.0, 2.0, 3.0);
	TS_ASSERT_VECTOR_EQUALS(ic.GetWindNEDFpsIC(), FGColumnVector3(1.0, 2.0, 3.0));
	TS_ASSERT_DELTA(ic.GetWindNFpsIC(), 1.0, epsilon);
	TS_ASSERT_DELTA(ic.GetWindEFpsIC(), 2.0, epsilon);
	TS_ASSERT_DELTA(ic.GetWindDFpsIC(), 3.0, epsilon);
	TS_ASSERT_DELTA(ic.GetWindFpsIC(), sqrt(5.0), epsilon);
	TS_ASSERT_DELTA(ic.GetWindDirDegIC(), atan2(2.0, 1.0)*180./M_PI, epsilon);
	double mag = ic.GetWindFpsIC();
	ic.SetWindDirDegIC(30.);
	TS_ASSERT_DELTA(ic.GetWindNFpsIC(), 0.5*mag*sqrt(3.0), epsilon);
	TS_ASSERT_DELTA(ic.GetWindEFpsIC(), 0.5*mag, epsilon);
	TS_ASSERT_DELTA(ic.GetWindDFpsIC(), 3.0, epsilon);
	ic.SetWindMagKtsIC(7.0);
	TS_ASSERT_DELTA(ic.GetWindNFpsIC(), 3.5*sqrt(3.0)*ktstofps, epsilon);
	TS_ASSERT_DELTA(ic.GetWindEFpsIC(), 3.5*ktstofps, epsilon);
	TS_ASSERT_DELTA(ic.GetWindDFpsIC(), 3.0, epsilon);
	}

[bcoconni]

Since the code is trivial, I'd rather move that definition into the header so that the C++ compiler can inline it when possible (see my other comment below):

Suggested change

	void FGInitialCondition::SetWindMagFpsIC(double mag)
	{
	SetWindMagKtsIC(mag*fpstokts);
	}

[seanmcleod]

Will do, although C++ compilers/linkers have been able to inline methods in .cpp files for many years 😉

[seanmcleod]

I'll add an additional test to the unit test.

[bcoconni]

Will do, although C++ compilers/linkers have been able to inline methods in .cpp files for many years 😉

OK I'm lagging many years behind 😄 but I've always had this idea that when the body of a method is given in the header then compilers would intepret that as an inline requirement (that they can happily ignore by the way). Anyway that doesn't hurt either way but having a one-liner body in the header saves the trouble of opening the .cpp file 😉

[seanmcleod]

Yep, and there are tons of other one-liners in that header file as well, so it'll match the pattern.

[bcoconni]

The getter and setter have dissimilar names: should be SetWindFpsIC() ?

[seanmcleod]

Yep, did wonder about that a bit. Matched the new setter to match the name of the existing setter that is identical except for having Kts in the name.

Looking at it again I'm actually inclined to rename the getter from GetWindFpsIC() to GetWindMagFpsIC() to make it clear it's returning the magnitude as opposed to a vector or some vector component.

[bcoconni]

Looking at it again I'm actually inclined to rename the getter from GetWindFpsIC() to GetWindMagFpsIC() to make it clear it's returning the magnitude as opposed to a vector or some vector component.

Looks good to me.

[bcoconni]

As per my comment above.

Suggested change

	void SetWindMagFpsIC(double mag);
	void SetWindMagFpsIC(double mag) { SetWindMagKtsIc(mag*fpstokts); }

[bcoconni]

@seanmcleod Tests are failing: it seems you've omitted changing GetWindFpsIC() to GetWindMagFpsIC() in some other unit tests.

[seanmcleod]

Yep, I had done a search for GetWindFpsIC() when I first changed the method name, but that was only in JSBSim, and JSBSim compiled fine locally. I thought my search had been across the whole source tree so didn't pick up the references in the unit tests, which I didn't compile locally.

[bcoconni]

All good 👍