Hi Andy, thank you for reviewing the patch. Andy Ross wrote: > Maik Justus wrote: > >> for my part the update of the yasim heli simulation is ready for >> cvs. Andy: therefore I want to ask you for a review of the patch >> and your agreement to add this to cvs or a list of necessary >> modifications or (hopefully not) a clear no. >> > > OK, here's a quick review of the stuff I don't like. There are no > comments here on the Rotor* files. Those are 100% yours and really > not my business to complain about. :) > > Explain why the changes to Surface.cpp are needed? The comments about > stall lift are incorrect -- YASim does not attenuate lift at stall to > zero. my impression was, that a stab is not producing forces perpendicular to its surface, when the wind is blowing perpendicular to its surface, as it is for the hstab for a helicopter in hover (valid only for helictopters with the hstab beneath the rotor). I think that this is to the fact, that "stallMul" becomes one for this case and therefore "stallLift = (stallMul - 1) * _cz * out[2]" becomes zero. This conditions will not occur in a plane and therefore this had no visible impact to planes. > And why do you need incidence values to be non-small angles? > If that's the requirement, then it would be better to have an > implementation that can rotate the surface orientation instead. Doing > it here loks like a hack. Do you actually need these changes for the > helicopter code? Why? > The fuselage is simulated as a open tube and in the bo the resulting drag was to small I think. Therefore I added a vertical stab to close the tube at the beginning. This vertical slab I realized by an stab with 90 deg. incidence. There should be many other ways to do this without a vertical stab, but this way was self-evident for me.
> The boundary changes in Wing.cpp don't make any sense to me. You are > adding two entries, but don't map them to anything. How is this > change not a complete (and hard to understand) no-op? > > The outer definition of the wing in Yasim is given by the innermost and outermost bound[]. But the bound[] are only defined at flapx start, flapx end, spoiler... and slat... .Therefore for an wing without flap, spoiler and slat the bound[] array is empty and no surface element is generated. E. g. for the bo105 hstab (without flap, spoiler and slat) this stab is not simulated in YASim. E. g. for the j3, with only one control subelement at the wing (<flap0 start=".31" end="0.85" lift="1.5" drag="1.4"/>), there is no surface generated for the outer 15% of the wing (the same for the inner 31%). > The changes to Model::calcForces() are just not acceptable. This > routine should be a short, top-level wrapper for the force calculation > (i.e. for each surface, set up parameters and calculate forces, then > do the same for each engine, etc...). You have dumped a *TON* of > quite clearly helicopter-specific logic right into the middle of this > function. (Amusingly, you also broke my comment that reads "end of > engine stuff" -- it now comes something like 100+ lines after the end > of the engines). Can't this go into one of the Rotor* files? At the > very least, get it out of calcForces(). > Yes, I will change this. The lines between the two comments // check,<if the engine... and // End of engine stuff will be replaced by one function call. The few remaining lines I will reduce (one of the for-loops can be included to the other for-loop) ...by the way, I think the comment that reads "end of engine stuff" is by me. > Also, I notice that I had a nit-picky comment at the top of > Model::initRotorIteration(). I know you read this comment, because > you modified the function. But you don't seem to have addressed the > concern. Either remove the comment if it is incorrect, explain to me > why it's not a problem, or fix the code. :) > Of course I didn't want to change your comment. Yes, there is some integration done in the "void Rotorpart::inititeration(float dt,float *rot)" called from this function. One is the integration of the "rotor orientation" by omega*dt for the 3D-visualization of the heli only. The other is the compensation of the rotation of the fuselage. The rotor does not follow the rotation of the fuselage. Therefore its rotation must be subtracted from the orientation of the rotor. I think, one could call this "unstiff problems". > The changes to RigidBody.cpp are just plain wrong. You appear to be > trying to handle a divide-by-zero condition by testing the > denominator. Which is sort of OK, if clumsy (AFAICT, neither of these > situations can occur in practice -- please explain why you needed to > fix these; I suspect you had some other bug). Unfotrunately, your > fallback cases treat the infinity that gets produced as being EQUAL TO > ONE! > For debugging the rotor code I had switched the floating point exceptions on. But I got floating point exceptions exactly in the modified functions directly after starting the program, probably due to uninitialized variables somewhere else. Maybe the exceptions were based on a bug in the heli simulation (due to the not invoked solver in heli simulation. I think I remember, that "setupWeights(true)" was missing there). I will set breakpoints to this functions to clarify this. (I just did a short test-run and did not get an exception). Likely you are right and the changes are plain wrong. > And, just in general, I wouldn't mind a little code cleanup. There > are several spots where you have commented code out instead of > removing it; I hope I only did this where I modified your code to illustrate the changes. > I see a few places where you have more than one blank > line in a row; your brace/whitespace/naming conventions > don't always match the surrounding code, etc... Yes, that should not be. I will try to find all these uglinesses. > I'm not a big zealot about > this stuff, though (except for the commented-out-code issue -- that > drives me nuts). > > will be removed, of course. > Andy > Maik ------------------------------------------------------------------------- Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys -- and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV _______________________________________________ Flightgear-devel mailing list Flightgear-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/flightgear-devel
