c172p-team / c172p

A high detailed version of the Cessna 172P aircraft for FlightGear
GNU General Public License v2.0
82 stars 43 forks source link

Issue 1330 #1332

Closed gilbertohasnofb closed 4 years ago

gilbertohasnofb commented 4 years ago

Closes #1330

The idea behind this PR is:

gilbertohasnofb commented 4 years ago

@wlbragg @dany93 and everyone else: ready for testing and merging

dany93 commented 4 years ago

Correct for the inverted directions.

Trim wheel: I think that the control step is too coarse: try stabilizing a horizontal flight to see.

In Models/c172p.xml, lines 5238 - 5247

        <action>
            <binding>
                <command>property-adjust</command>
                <property>controls/flight/elevator-trim</property>
                <factor>0.001</factor> <!-- 0.01 -->
                <min>-1</min>
                <max>1</max>
                <wrap>0</wrap>
            </binding>
        </action>

The initial <factor> is 0.01. Changing it for 0.001 is good IMO. (this is the step I have on my joystick binding file)

@wlbragg, can you confirm that I did it the right way?

In this case, displaying three decimal digits (instead of two) on the hover indication would be better too. I'm not used to that, but

        <hovered>
            <binding>
                <command>set-tooltip</command>
                <label>Elevator trim: %3.3f</label> <!-- %3.2f -->
                <tooltip-id>pitch-trim</tooltip-id>
                <property>controls/flight/elevator-trim</property>
            </binding>
        </hovered>

(with %3.3f instead of %3.2f) does the trick.

gilbertohasnofb commented 4 years ago

@dany93 just to be clear, I have not changed the trim property itself, just how fast the animation of the trim wheel is. I can of course adjust that in this same PR. @wlbragg can you give us your thoughts?

dany93 commented 4 years ago

In this case, it was like that before. I had never noticed it because I use my joystick for trim control. And the trim wheel is not easily visible. Weird that nobody else noticed it....

I give my opinion but I'd prefer that you and @wlbragg are convinced if you do it. Can you try stabilizing in flight?

legoboyvdlp commented 4 years ago

I agree -- also throttle; setting by 5% seems a bit fast.

wlbragg commented 4 years ago

I like it right where it's at. It seems OK to me. I defiantly wouldn't want to go coarser than it is now but i don't seem to have a hard time tuning it where it is at. I just tried it at 0.001 and I feel it is too fine and takes to long to make any change. Keep in mind this is with a mouse, It may be that with the JS it is more sensitive and the JS needs to be set finer. But with a mouse it's really slow at .001. Something else I don't like is the rudder trim. Trying to keep the mouse on the lever and scrolling the wheel doesn't work well. I think the text legend needs to also be hot.

gilbertohasnofb commented 4 years ago

I just tried it at 0.001 and I feel it is too fine and takes to long to make any change.

@wlbragg I have to say I agree with @dany93 on this one. Our current value is 0.01, I tried halving it to 0.005 and I still struggle to precisely trim the aircraft in air. I understand that it takes long to make large changes, but the question is do pilots really need to make such large changes in RL? Actually, is the range of our trim too large perhaps? Regardless, I am definitely up for lowering it substantially, possibly to 0.002.

also throttle; setting by 5% seems a bit fast.

@legoboyvdlp I tested it at 2% and it feels much better. Much better control over mixture and throttle and it still takes a reasonable time to go from open to full.

I can push these changes for you all to test if we are in the same boat.

dany93 commented 4 years ago

@wlbragg wrote

I just tried it at 0.001 and I feel it is too fine and takes to long to make any change. Keep in mind this is with a mouse, It may be that with the JS it is more sensitive and the JS needs to be set finer. But with a mouse it's really slow at .001.

I have in mind that I rarely have to change the elevator trim by more than 0.1 or 0.2 (0.3 at final approach). Which does not take that long, and is to be made progressively. Controlling with a mouse or a joystick should not be different if one wants to test the trim. The difference is that, with a mouse, you can stabilize at any value different from 0, no trim needed. With a joystick, the springs hold it at zero when released and you need a trim to stabilize.

With my JS trim at 0.001 step, I can stabilize horizontal flight during more than one or two minutes with no JS intervention. 160 hp engine (read in the Internal Properties) 2276 RPM 106 kts elevator trim 0.096 aileron trim 0.019 Stability: vertical speed oscillates +/- 20 ft/mn.

gilbertohasnofb commented 4 years ago

For me, we can have the trim much much finer as Dany is suggesting, even at 0.001 as he says. I will push this modification shortly together with the throttle and mixture with steps of 2% instead of 5% so that everyone can test it.

aileron trim 0.019

What aileron trim? I thought our aircraft did not have an option for that.

Something else I don't like is the rudder trim. Trying to keep the mouse on the lever and scrolling the wheel doesn't work well. I think the text legend needs to also be hot.

We might want to make that one finer too. Also, I do not understand what you mean by hot legend.

dany93 commented 4 years ago

@gilbertohasnofb wrote

What aileron trim? I thought our aircraft did not have an option for that.

Correct on our aircraft. But for convenience, I coded aileron, elevator and rudder trim controls by buttons in my Joystick binding file. That's cheating, but Joysticks go back to 0 when you loosen them (springs) and this is much more uncomfortable than in reality. In reality, it is not so strong and you don't feel this kind of "notch" at neutral. Anyway, even in reality, having neutral controls is very comfortable and more accurate. For the aileron (and rudder trim if there is no), this is done by mechanics on the ground, compromise at a given cruise airspeed.

I gave you the aileron trim to help if you wished to try this way. Accessible by setting it in the properties. For a given rpm and airspeed (close to the one I gave), you even do not need changing the aileron trim value.

Also, the possibility to loosen the JS controls and have a stable trajectory is particularly interesting for accurately doing tests such as drag measurements.

wlbragg commented 4 years ago

I'm OK with whatever you all decide. .002 would be my choice over .001. My thoughts on @dany93 argument of trimming only .1 to .3 during a flight kind of supports the value being .01 and not .001. Theoretically wouldn't it take 10 turns or 10 to 1 to turn from .1 to .2 at .01, but 100 turns at .001? With a matching "step" setting of course. That is what I feel on my mouse when the setting is at .001. Are you all using the mouse for this, except @dany93? 100 turn to get a 1% trim is excessive. Let me try it again, I was in a real hurry when I tried it last time.

I do not understand what you mean by hot legend.

Meaning the "hot" spot is the knob and it appears to scroll off the mouse when turning the mouse wheel and then wont scroll anymore without moving the mouse back over the knob. So if you make an invisible hot spot (that's what I really meant), the size of the texture mesh for the entire trim graphic or close to it. Not a must I don't use it that much and it's not that hard to use. I'm not usually trimming the rudder but maybe once and that would be at a low workload time where I can afford some attention.

wlbragg commented 4 years ago

After reviewing it again and using it to trim the aircraft I am OK with .001 or .002. It is actually way better. The first time I was in a hurry and it appeared it was taking way to long to move it to where it was effecting the aircraft. But from a "sane" starting trim it is actually a fine adjustment you need and being coarse just makes it harder ti get the trim correct. It's possible we will get some negative feedback on this at first introduction to those that are used to the other setting. But I believe the "finer" grain is better. I had been testing MS2020 and that trim is similar to ours now and not at all easy to set. It is to coarse..

dany93 commented 4 years ago

@wlbragg wrote

Theoretically wouldn't it take 10 turns or 10 to 1 to turn from .1 to .2 at .01, but 100 turns at .001?

(if I understand well what you wrote...) Going from 0.1 to 0.2 trim (= 0.1 difference) takes 10 steps at 0.01/step, 100 steps at 0.001/step. One scroll-wheel notch gives one step (0.01 or 0.001). Which makes 10 or 100 scroll-wheel notches, not 10 or 100 scroll wheel turns.

The best and easiest is to rotate the scroll-wheel and observe the trim values in the sim.

dany93 commented 4 years ago

@wlbragg wrote

Something else I don't like is the rudder trim

I almost never use the rudder trim setting. I had to change it once on the P51D for settings of the propeller effects. But in fact, these effects were way too strong, which needed a lot of rudder (or trim) to keep the slip ball centered. Since that, I usually set a (weak) compromise value in the -set.xml file and that's enough. At climbing, full thrust, one needs the rudder but only temporarily, and at cruise the compromise (fixed) value is sufficient.

wlbragg commented 4 years ago

Right, I wasn't trying that hard to see the relationships, turns, notches, etc., I was more generalizing. I think we're on the same page. I'm good with the changes as is. My bottom line is I can turn the wheel starting at a "sane" beginning value and get it trimmed without having to excessively take my attention away from flying.

I did think of a really nice addition we can make in time. I really hate having to change view to set any setting. The options are a quick key (I don't know if trim has any). I of course can use my JS, but for those on mouse I want to make a GUI popup like the Shuttle uses for many things (canvas) with the actual animated graphical mouse wheel for trimming so you don't have to change view at all yet it will feel like your actually doing it same as looking down at it because you see the graphical visualization of it in a popup.

gilbertohasnofb commented 4 years ago

Ok, so I pushed my changes, please see what you all think. This is what I did:

@wlbragg I see what you mean about the hotspot now, and that is a good idea, but that would require some 3D modelling. If you want to do it, then we should add a hot spot behind the whole rudder trim, not only a cube at the centre, since users can still try to scroll above the handle. But I am fine with how it is as well and we can tackle this in the future at some point.

dany93 commented 4 years ago

@wlbragg wrote

I did think of a really nice addition..... I really hate having to change view to set any setting..... for those on mouse I want to make a GUI popup...... with the actual animated graphical mouse wheel for trimming

Seems a nice idea. But one still has to see the hot spot to be clicked (e.g. the trim wheel) to click on it. Not easy unless with a large zoom angle or a kind of temporarily zooming.

dany93 commented 4 years ago

@gilbertohasnofb Tested. From my point of view, everything is fine. (except rudder trim, not tested because I don't know where it is)

Nice work, which has gone farther than you thought. Thank you.

wlbragg commented 4 years ago

Seems a nice idea. But one still has to see the hot spot to be clicked (e.g. the trim wheel) to click on it.

I would use a quiick key to open it.

gilbertohasnofb commented 4 years ago

@dany93 Thanks for testing!

(except rudder trim, not tested because I don't know where it is)

This is enabled in the C172P > Aircraft Menu dialogue.

@wlbragg When you have a chance, would you check the modifications again just so we are sure everyone is happy with the results before merging? Otherwise this is ready to be merged

dany93 commented 4 years ago

Rudder trim: I tested it.

It has a very weird action. It makes the aircraft turn, but not like the rudder should do. The slip ball almost does not change, differently from an action on the rudder. In FG, it should change the rudder trim controls/flight/rudder-trim. Very simple. Instead, it changes controls/flight/rudder-trim-knob, fcs/yaw-trim-cockpit, and its yawing moment seems added to yaw-trim-sum by a very weird an inappropriate manner. I think it does not act like the rudder (+rudder-trim) should do. I don't understand why this weird way of adding a rudder trim has been used.

In reality, the rudder trim acts by changing the "neutral" position of the rudder (thanks to the airflow on the rudder-trim tab) when there is no action on the yoke. At the end, the yawing moment is always done by the rudder position (slightly deflected here). Not some other extra "thing".

wlbragg commented 4 years ago

We definitely need to change it then, I don't recall who did the code I know I did the modeling and @gilbertohasnofb did the textures. I may have done the code as well and didn't understand the FDM properly.

dany93 commented 4 years ago

In the FDM, controls/flight/rudder-trim is added to controls/flight/rudder by the intermediate of the fcs. This sum forms the Yaw Trim Sum.

To make it simple, all that the lever has to do is changing controls/flight/rudder-trim

At first view, it only should remain

            <summer name="Yaw Trim Sum">
                <input>fcs/rudder-cmd-norm-filtered</input>
                <input>fcs/yaw-trim-cmd-norm</input>
                <clipto>
                    <min>-1</min>
                    <max>1</max>
                </clipto>
            </summer>

(see the <summer name="Pitch Trim Sum"> and <summer name="Roll Trim Sum"> to confirm)

Setting 0.13 to the rudder trim should have the same effect as adding 0.13 to the rudder control.

legoboyvdlp commented 4 years ago

All looks good here :+1:

gilbertohasnofb commented 4 years ago

@dany93 Ok, I found three entries where controls/flight/rudder-trim-knob was mentioned in c172p.xml and I changed it to controls/flight/rudder-trim. Could you please take a look if this takes care of the issue with the FDM?

dany93 commented 4 years ago

@wlbragg controls/flight/rudder-trim After more thinking, your initial code with rudder-trim-knob did the job, sorry for having been too harsh. However, by an inappropriate manner. Moving the rudder-trim knob without seeing the controls/flight/rudder-trim value change was confusing. Moreover, the code with controls/flight/rudder-trim is simpler. Not only in accordance with usual way.

@gilbertohasnofb , @wlbragg It works well like that. However, some cleaning remains to be done.

Despite these deletions,

(Probably not exhaustive....)

wlbragg commented 4 years ago

After more thinking, your initial code with rudder-trim-knob did the job, sorry for having been too harsh.

No Worries!

It might be better to have it at 0 at start with the rudder trim option (@wlbragg ?).

Would most people more than likely have to adjust the knob then at startup? Or would it be best to have the knob position match the .02 at start up? If .02 is a good trim I would feel funny leaving the plane out of trim without some kind of notice, although we don't do that with the elevator trim. The elevator trim is expected to be change thought in most every flight.

gilbertohasnofb commented 4 years ago

@wlbragg @dany93 could you guys take a look at this and push fixes into this branch? I think I know less than both of you about the rudder trim issue.

Would most people more than likely have to adjust the knob then at startup? Or would it be best to have the knob position match the .02 at start up? If .02 is a good trim I would feel funny leaving the plane out of trim without some kind of notice, although we don't do that with the elevator trim. The elevator trim is expected to be change thought in most every flight. Merge state

The state of the rudder trim should be saved if the option of saving states is on. The idea is that the trim tab will be left in the same position as the last pilot, and it's part of the realism to check for that. But In the case of the rudder trim, we need to reset it to 0.0 whenever the option of using rudder trim in the aircraft dialog is disabled, since the pilot will not have access to it any longer. Makes sense?

wlbragg commented 4 years ago

The state of the rudder trim should be saved if the option of saving states is on.

OR

But In the case of the rudder trim, we need to reset it to 0.0 whenever the option of using rudder trim in the aircraft dialog is disabled

I think you meant .02?

Yeah, I got that, so you think it is OK to set it to .02 when rudder trim is disabled and always back to 0 when rudder trim is enabled? If so then I wonder if we need a separate property for the adjustable rudder trim so it can have its own persistence separate from the auto trim. Then it would work like thi... Select auto trim and trim is set to .02 always.

Enable manual trim and use its isolated property so it can be persistent and it will be whatever that last value was set at. OR Enable manual trim and it is always set to 0. OR Enable manual trim and it is always set to 02.

The above is settings are on the fly.

Then we have the "on startup" and of course auto trim will always be at .02. But manual trim could either be persistent or always set to 0 or always set to start at .02.

These are all the options we have, what do we want?

dany93 commented 4 years ago

Select auto trim and trim is set to .02 always.

Yes, if possible (we are on the fly). Or (in fact), the value in c172p-set.xml. Otherwise, it will be set at next startup.

Enable manual trim and use its isolated property so it can be persistent and it will be whatever that last value was set at.

Uselessly complicated. If one clicks to enable the manual trim, it can be set to 0. This is a new configuration.

Enable manual trim and it is always set to 0.

Yes

Enable manual trim and it is always set to 02.

No. It can (should) be set with the trim lever. Otherwise, complicated because you would have to read the c172p-set value, which is an approximate compromise, not that important. And useless for manual-trim.

The above is settings are on the fly.

Yes.

Then we have the "on startup" and of course auto trim will always be at .02.

Yes Yes! Or the value in c172p-set.xml

But manual trim could either be persistent or always set to 0 or always set to start at .02.

If no too complicated, the best would be persistent if the manual option is kept from one session to the next. Otherwise (still if manual option is kept from one session to the next), 0 is the best and most simple.

I noticed that, since my last proposed cleaning of files (or maybe not due to it), disabling the rudder trim (i.e. changing from manual to disabled = auto in flight) resets it to 0. Which is perfectly acceptable because it is only for the current session. Next start with "disabled" (= auto trim) will be with 0.02 (or the value in c172p-set.xml).

dany93 commented 4 years ago

@wlbragg wrote:

If so then I wonder if we need a separate property for the adjustable rudder trim so it can have its own persistence separate from the auto trim

For what it's worth, kind of template for the principles... A possible way (??) might be in c172p-set.xml

    <controls>
        <flight>
            <rudder-trim-preset type="double">0.02</rudder-trim-preset>

In c172p.nas (if auto trim) setprop("/controls/flight/rudder-trim", getprop ("/controls/flight/rudder-trim-preset")); or (if manual, new check or non-persistent at startup) setprop("/controls/flight/rudder-trim", 0); or (if manual, persistent, already checked at startup) setprop("/controls/flight/rudder-trim", "the persistent value");

This way, the FDM is unchanged and as usual. Also, the pre-set value in c172p-set.xml is easy to understand.

gilbertohasnofb commented 4 years ago

@dany93 would you like to push the necessary modifications to the rudder trim so that we can merge this?

dany93 commented 4 years ago

I have nothing to push. My previous message was just indications to (maybe, hopefully) help @wlbragg if you want to include this features in the code. But, as I wrote it, I'm not sure of their validity, and you can see part of them are only schematic. And @wlbragg can have better ideas, he did most of this code. I discovered this feature by reading this issue. Among other things, I have no practice of the persistent values and options.

gilbertohasnofb commented 4 years ago

@wlbragg would you like to work on the rudder trim feature on this PR?

wlbragg commented 4 years ago

Yeah, I'll finish this up. I have my hands full at the moment with out of town guests, but will try to squeeze it out in the next 5-7 days, if that is OK?

gilbertohasnofb commented 4 years ago

Absolutely, there is no rush. Hope you are having a great time with your guests!

wlbragg commented 4 years ago

@dany93

I wasn't understanding closely what the changes were here. But I understand we basically removed rudder-trim-knob as the controlling property for this.

I'm trying to finish these last few PR up.

You posted

Despite these deletions,

the controls/flight/rudder-trim-knob still appears in the Internal Props,

I did a find-in-files search and below are the remaining uses of the "rudder-trim-knob" property. I assume the sound is OK or have we completely removed the "rudder-trim-knob" property and the sound should be tied to something else?

The setting of the "rudder-trim-knob" property to 0 in the dialog should be removed as well? Actually it will be replaced by the proper trim property which is what now? Is is simple controls/flight/rudder-trim?

Searching 915 files for "rudder-trim-knob"

/mnt/IDrive/FG/Aircraft/Development Aircraft/c172p/c172-sound.xml:
 1296              <mode>in-transit</mode>
 1297              <path>Sounds/rudder-trim.wav</path>
 1298:             <property>/controls/flight/rudder-trim-knob</property>
 1299              <position>
 1300                  <x>-0.3660</x>

/mnt/IDrive/FG/Aircraft/Development Aircraft/c172p/gui/dialogs/aircraft-dialog.xml:
  367                      <script>
  368                          if (!getprop("/sim/model/c172p/ruddertrim-visible")) {
  369:                             setprop("/controls/flight/rudder-trim-knob", 0);
  370                          }
  371                      </script>

3 matches across 3 files
wlbragg commented 4 years ago

nasal errors still appear (although not sure they are connected with this issue) [ALRT]:nasal ERROR: Cannot add listener to tied property /controls[0]/flight[0]/rudder-trim[0]

I don't think this is anything.

wlbragg commented 4 years ago

I already removed the other code you suggested above and changed the tool tip string formatting

<label>Elevator trim: %3.3f</label> <!-- %3.2f -->

but haven't pushed it yet.

wlbragg commented 4 years ago

Marked for deletion

c172p.nas, line 321
# setprop("/controls/flight/rudder-trim-knob", 0.0);

In reality, I think this will change to the rudder trim persistent property for the manual trim. The setting of the automatic trim will be done at startup or in the dialog when the change is to use "no ruder trim slide".

In c172p-set.xml, I had set the rudder trim at 0.02 as an approximate compromise to have the slip ball centered at cruise

0.02

It might be better to have it at 0 at start with the rudder trim option (@wlbragg ?). Otherwise, the effect is (was, when I did it?) not only an approximate compromise without rudder trim setting in the cockpit, but also very weak. An possibly depending on 160 or 180 hp...

I will finish all this up as well and this should be all to close this issue up.

wlbragg commented 4 years ago

@gilbertohasnofb @dany93 I don't think you all understood why I did what i did with rudder-trim-knob and I certainly didn't defend why I did it that way, because I didn't remember why. It was set for the animation of the knob graphics and then mapped to the rudder-trim property. By eliminating it and using the rudder-trim property directly the rudder trim knob no longer works correctly and I have to change the range and scale of that animation. I don't know if that is possible or not. I guess I can convert or scale the rudder-trim range to match the rudder trim knob animation directly? I just discovered this and ran out of time for today. I will look at it again tonight or tomorrow.

wlbragg commented 4 years ago

I think I figured out how the persistence of the rudder trim will work. We set it to 0 in the set file. On load, if ruddertrim-visible is false, (it is auto), it gets changed to .02 If not it remains at 0, ("We set it to 0 in the set file"), unless it was made persistent by "switches_save_state" in which case it will be whatever it was set at when exiting last. There is a listener tied to the ruddertrim-visible and if it is triggered it sets it to 0 or .02 accordingly.

wlbragg commented 4 years ago

I guess I can convert or scale the rudder-trim range to match the rudder trim knob animation directly?

OK, I was able to fix this in the animation.

A couple more questions about it however. 1) Should the ruddertrim-visible property be persistent always if you exit the sim with it active or only if you check "Switches Save State"? 2) Depending on (1) above, should the rudder trim setting be persistent?

Choices for (2) are... if the ruddertrim-visible property is persistent always if you exit the sim with it active then should the actual rudder trim be persistent always or only if you have also selected "Switches Save State"?

wlbragg commented 4 years ago

OK I think this one is done. I am happy with the persistence logic. It is as follows... Auto rudder trim is always set to .02 whether starting the sim or changing it in the GUI. Manual rudder trim visibility choice is always persistent. If exiting with rudder trim visibility set to true then the rudder trim setting will either be set to 0 at the start of next session or set to the last setting if the "Switches Save State" is also checked. If the rudder trim visibility GUI choice is changed from auto to manual the rudder trim is always set back to 0.

Please verify before merging... 1) all @gilbertohasnofb changes for elevator trim are still in this and correct 2) rudder trim knob change sound is OK 3) rudder trim is functioning correctly 4) rudder and elevator trim tool tip formatting correct 5) rudder trim and elevator trim persistence is as intended

I think that is it, I'm moving on to #1327 and #1335

gilbertohasnofb commented 4 years ago

A couple more questions about it however.

I think the rudder trim option should be persistent between sessions despite the switches save state, since that's an aircraft option similar to having the GPS or bush wheels which, I believe, persist between sessions despite the save state option (which is correct).

If that option is ON, the rudder trim value should be persistent, otherwise it should go back at zero. Also, whenever the rudder trim option is removed, it's value should also be reset (since the user won't have the trim to change it back).

What do you think?

Edit: I see this is exactly what you wrote, so it should all be good. I will test this and merge it now.

gilbertohasnofb commented 4 years ago

@wlbragg it all works exactly as it should, thanks for this

wlbragg commented 4 years ago

Thanks @gilbertohasnofb !