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. 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 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 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(). 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. :) 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! 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 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... I'm not a big zealot about this stuff, though (except for the commented-out-code issue -- that drives me nuts). Andy ------------------------------------------------------------------------- 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 [email protected] https://lists.sourceforge.net/lists/listinfo/flightgear-devel
