#2521 Submodel translation and rotation offsets are in the wrong order

[Colin Geniet]

In submodels definitions, rotation offsets are applied after translation offsets. Thus

<offsets>
  <x-m>-1</x-m>
</offsets>

moves the starting point 1m forward, but

<offsets>
  <x-m>-1</x-m>
  <pitch-deg>90</pitch-deg>
</offsets>

moves the starting point 1m up, and pointing up. (I would expect 1m forward and pointing up.)

This is weird, and doesn't correspond to what is stated in README.submodels

[Colin Geniet]

The issue seems to be in FGSubmodelMgr::getCartOffsetPos.
The following (line 739) applies the yaw/pitch offsets to the translation offsets.

    // postrotate by any pitch/heasing/roll offset
    hlTrans *= SGQuatd::fromYawPitchRollDeg(sm->yaw_offset->get_value(),
                                            sm->pitch_offset->get_value(),
                                            0.0);

After removing these lines, the submodel initial position is as expected, and the initial direction is still correct.

[James Turner]

Problem I can imagine here: if we make this change (fix), what % of existing subvmodles do we break? Probably not very many, but probaby > 0. So we need to decide if we introduce a new property, or a compatability flag, I guess.

[Colin Geniet]

I think it is a regression introduced by this commit:
https://sourceforge.net/p/flightgear/flightgear/ci/ea336af4dc8d386c37316f385a77b8ac0cd5319c/

In which case I suspect changing it would repair more submodels than it would break.

[James Turner]

Reading that commit: wasn't the intention of the author, exactly to make it that pitch/yaw was applied to the offset? The commit is literraly titled 'Submodel offset supports pitch/yaw values', after all.

Are you thinking that commit was just wrong, since it broke existing models? Or that it was the correct idea, but implemented the wrong way?

[Colin Geniet]

This is the mailing list thread behind this commit
https://sourceforge.net/p/flightgear/mailman/flightgear-devel/thread/69066C23-C794-470D-96C7-D7B892F965D6%40flightgear.org/#msg36646104

[Colin Geniet]

I think I understand.

The bluebird FDM does something really strange: property /orientation/pitch-deg is directly tied to the elevator input, and remains 0 (assuming no input) when pitching due to ground contact. Then, the bluebird computes pitch angle due to ground contact, and applies it to the 3D model as an animation.
Because of this, the aircraft local coordinate system is wrong (it is not aligned with the model).
One manifestation is that submodels do not appear at the correct location.

Then, instead of fixing the FDM, the bluebird developer tried to manually compensate for this issue on the side of submodels, by applying the 'rotation due to ground' angle. This requires to do a rotation centered on the aircraft position (and not on the submodel position). Which is why this change was requested.

So I stand by my claim that this change (the 4 lines posted above) is wrong. It was requested to fix a manifestation of a deeper issue specific to the bluebird, and it is incorrect for just about every other aircraft (for instance it breaks flare submodels on any military aircraft launching flares with significant angle offset, of which there are many).

[James Turner]

Thanks for the clear explanation, that indeed makes sense. In the context of those changes, we were only considering the Bluebird for sure, I don't fly any other aircraft with submodels so never noticed the regression.

What I'd like to propose here is that we revert the line as you suggested, but then also try and capture the behaviour in some unit-tests. I can setup a skeleton but help adding test cases (like the appareny-wind one from a few months ago) would be good.

The only problem that will leave is the Bluebird will be broken again; I don't know if anyone cares to look into its FDM, which does indeed sound like it's taken straight from the UFO.

[Colin Geniet]

Here's a start: https://sourceforge.net/p/flightgear/flightgear/merge-requests/237/
And already some results: it turns out that attribute AIBase::vs is supposed to be in fpm, but AIBallistic misuses it as fps.

[Colin Geniet]

The only problem that will leave is the Bluebird will be broken again

Contacting the maintainer would be a start.

If I understand correctly the ufo FDM has a C++ part and a Nasal part. Are the orientation properties controlled from C++? That would indeed make it hard to fix this from the bluebird side.