Harald67 / c150

Flightgear Cessna 150
4 stars 1 forks source link

New starter system, split lever sound #38

Closed ghost closed 8 years ago

ghost commented 8 years ago

Request to merge ;) Could you cut out the breakers and give them the names according to the code and then uncomment teh code? That should make them work ;)

Harald67 commented 8 years ago

I've not yet merged the pull request, but I think there are some problems:

ghost commented 8 years ago

_is the starter key working as before ? I think I will not turn to the right when pressing 's' (but this is not really clear for me). -> Sorry, that's a bug I forgot, will fix in the next commit you added a second master-alt pick animation instead of changing the already existing one. -> should be changed in 431a6f0 was beginning of breaker code you've redefined the 's' key, but this should be done with a redefinition of the controls.startEngine() nasal function instead so that the engine can be started with any hardware interface. -> okay, that's something I didn't know, thought this was equal for all hardwares_ if you change the engine sound then you must adapt the picth so that it matches the rpm. -> I will ;) i'm not sure why you changed the empty weight (even if there are numbers around 1060 floating around), you must not forget the weight of the equipment (radios, etc.). -> Oops, forgotten it, I just thought 1111 was very high (a 4-seat DR400 is 1212) and found a number of 1060, but as you've explained I'll change it back :D

ghost commented 8 years ago

Where to change controls.startEngine??

ghost commented 8 years ago

Commit 3d31438 should fix empty weight engine sound

ghost commented 8 years ago

@Harald67 could you help me how to change controls.startEngine pls?

Harald67 commented 8 years ago

in c150.nas you can add a function like

controls.startEngine = func(n) { setprop("/controls/engines/engine/starter-key", n); }

That way we do not need to redefine the standard 's' keyboard binding, so if someone is calling the startEngine function from a joystick button definition if will still work.

ghost commented 8 years ago

Should be fixed with last commit

ghost commented 8 years ago

Shall I merge this now?

gilbertohasnofb commented 8 years ago

@D-ECHO just saying, but I think we should adopt the policy of never merging our own PRs (that's at least how we do in the c172p and c182s projects). The idea is that a PR should be revised, and it's much easier for a different person to spot mistakes and bugs than the one who implemented it. Also, there might be things in your way of testing that are different than mine (maybe we tend to use different weather scenarios, maybe airports at different altitudes) which can easily make a bug spotable by one while not by the other. So even if people are slow for reviewing and testing, I would say to really wait for it, nobody will forget any PR behind, ok?

gilbertohasnofb commented 8 years ago

Also, I believe this PR broke two sounds that I had implemented:

ghost commented 8 years ago

ok sry will revert in two weeks when back


Gesendet mit der Telekom Mail App http://www.t-online.de/service/redir/email_app_android_sendmail_footer.htm

--- Original-Nachricht --- Von: Gilberto Agostinho Betreff: Re: [Harald67/c150] New starter system, split lever sound (#38) Datum: 12.08.2016, 22:21 Uhr An: Harald67/c150 Cc: D-ECHO, Mention

Also, I believe this PR broke two sounds that I had implemented:

* the parking brake sound is being used for the primer. I don't think a
  primer lever would make such a sound, so I would like that being
  reverted
* the engine sound is really lower in pitch, it sounds like the engine
  is about to die when at idle. Please revert to the previous sounding
  setting

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Harald67/c150/pull/38#issuecomment-239550425 , or mute the thread https://github.com/notifications/unsubscribe-auth/ALtQ-ClRDGVGun7CCdBPInzuSSbwMdtnks5qfNXjgaJpZM4Ji3nJ .

gilbertohasnofb commented 8 years ago

I will do it myself then

gilbertohasnofb commented 8 years ago

Another issue that I believe was caused by this PR: if you turn the engine on then flick the landing light switch, you get an error message about circuit breaks, which haven't been implement yet:

fgfs-screen-001

@Harald67 would you mind taking a look at this? I am leaving now and I won't be back until tomorrow night

gilbertohasnofb commented 8 years ago

@D-ECHO One more thing about PRs: please do not push a lot of unrelated work in a single PR because it makes it very confusing to review and even worse to revert something. In this PR you did modifications to the windows (3D model + animations) then worked with sound (sounds xml, wav files, a tiny bit of Nasal code), then added circuit breaks (xml). This should have been done in 3 PRs. And it's also nice if you open an issue before (such as you did for the window), then create a PR which once merged will close that issue, as this keeps things much more organized.

Please do not take these comments the wrong way, I am glad to see contributions to any project I participate, I just want to share these "guidelines" which make life easier for all of us, ok? :smile: :cookie:

Harald67 commented 8 years ago

The landing light problem was already there, I'll adjust the power consumption.