FGLGear doesn't need to inherit from FGSurface.

[bcoconni]

As I exposed in #883 (comment), FGLGear does not need to inherit from FGSurface. This PR introduces a few modifications to remove this inheritance without impacting the features provided by FGSurface.

The PR shows that:

• FGLGear does not use any methods that it inherits from FGSurface. Nor any code is using these inherited methods from anywhere else in JSBSim code.
• isSolid, maximumForce and rollingFFactor need not being members. In 2 cases, they do not even need being variable as they are used at a single location.
• bumpiness is not used anywhere.
• The only member that needs to be moved from FGSurface is staticFFactor. But it merely contains the value read from FGGroundReactions.
• The members staticFCoeff and dynamicFCoeff need not being in FGSurface as FGLGear is the only class that accesses them.

EDIT: The arguments above that are stroke out happened to be irrelevant as has been shown in the discussion below. The other arguments, including those exposed in #883 (comment), are still applicable.

[codecov]

Codecov Report

Merging #884 (a2c293e) into master (92fa1ea) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #884      +/-   ##
==========================================
- Coverage   23.06%   23.05%   -0.01%     
==========================================
  Files         167      167              
  Lines       19611    19619       +8     
==========================================
  Hits         4524     4524              
- Misses      15087    15095       +8     

Impacted Files	Coverage Δ
src/models/FGGroundReactions.cpp	30.27% <0.00%> (ø)
src/models/FGLGear.cpp	0.00% <0.00%> (ø)
src/models/FGLGear.h	0.00% <ø> (ø)
src/models/FGSurface.h	0.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[ermarch]

Not that I'm against refactoring JSBSim but let me explain what the original intention was:

The gears define friction coefficients but in reality this is only about rubber against asphalt or concrete. But even that is not exactly clear. Fortunately their friction coefficients are close enough you will hardly notice the difference.

But in reality you should take into account the surface properties of both the aircraft component (tires, skids, floats, skis, metal skin, cloth) and the ground surface which could be concrete, asphalt, grass, dry mud, moist mud, wet mud, snow, dry ice, wet ice, water)

This does not only affect friction factors but also impact-values like pressure resistance. Which in turn could be used for crash detection for example.

This is what the surface class was meant to be turned into.

[bcoconni]

As I mentioned above: "This PR introduces a few modifications to remove this inheritance without impacting the features provided by FGSurface."

This PR does not remove the features you mentioned, it is actually showing that the same result can be obtained without recurring to the inheritance of a class. Indeed FGLGear uses almost none of the features of the class FGSurface. Actually FGLGear just reads the friction values and pressure resistance from FGGroundReactions. The machinery of class inheritance is not needed for such a basic task.

This does not only affect friction factors but also impact-values like pressure resistance.

This feature is unaffected by this PR.

Which in turn could be used for crash detection for example.

At best crash detection and pressure resistance are loosely linked. As @seanmcleod mentioned in #819 (comment), one leg of the 747 main landing gears can absorb the entire aircraft landing load. This means that the runway must also be designed to sustain that exact same load. However a c172 will be smashed on such a runway before it reaches a fraction of that runway pressure resistance. So you cannot tell from a runway pressure resistance if a landing impact will result in a crash, a hard landing or a soft landing.

[ermarch]

"This PR introduces a few modifications to remove this inheritance without impacting the features provided by FGSurface."

You wouldn't know that since you removed the call to FGSurface::bind()
This exposed properties, for nearly 10 years, that are now not exposed anymore.

I would strongly advice to reject this pull request as it not well thought trough.
Please don´t try to fix something that ain't broken.

[bcoconni]

You wouldn't know that since you removed the call to FGSurface::bind() This exposed properties, for nearly 10 years, that are now not exposed anymore.

Good catch ! The call to FGSurface::bind() has been removed but that also removed some properties. This is now fixed in the PR : I've added the members isSolid, maximumForce, rollingFFactor and bumpiness to FGLGear and restored their corresponding properties.

Following this update, the PR still removes the inheritance of FGSurface and this time should have no impact on existing features.

I would strongly advice to reject this pull request as it not well thought trough. Please don´t try to fix something that ain't broken.

Come on ! You're overreacting.

[ermarch]

Okay, this looks better already.
I'm still not seeing the need for removing the class but at least it's functionality is copied over.

As FGSurface is only being used by FGGroundReactions after this patch you could do the same there.

Erik

[bcoconni]

Okay, this looks better already.

Thanks.

I'm still not seeing the need for removing the class but at least it's functionality is copied over.

I can see at least 2 reasons to remove FGSurface:

1. In its current state, the code is misleading as one might think that the ground height can/could be/should be computed in FGSurface::GetBumpHeight instead of FGGroundCallback as has been exemplified by the PR User #883. When code is misleading/confusing then it must be fixed.
2. FGSurface is what Fowler & Beck describe as the "Lazy Class" code smell in their book "Refactoring: Improving the Design of existing Code" so the code should be refactored.

As FGSurface is only being used by FGGroundReactions after this patch you could do the same there.

You're exactly right and this will give a third reason why FGSurface should be factored out.