On 04/05/2011 02:20 AM, Ricky wrote: > Consider the following replacement for the section under consideration: > --- > } > // factor the distance into the displacement vector. This will get us > // equally visible movements for both close and far away selections. > F32 min_dist = sqrt(fsqrtf(min_dist_squared)) / 2; As fsqrtf(min_dist_squared) isn't used except for computing this expression, it might make sense to write it in one line like this, thus saving one variable. However, I do not think doing so solves the naming problem. Note how min_dist now stores the exact value we were unsure how to call. And because it's an order-four root of min_dist_squared, min_dist is a misnomer for it. The only obvious way to avoid having to name that value is to move the call to the outer sqrt into the arguments of the following setVec. That however means it has to be called three times, which makes it a bit silly considering the overall intention of your change is optimization. Off course, two of the three calls might be automatically optimized away by the compiler, as the sqrt function should of course be referentially transparent <http://en.wikipedia.org/wiki/Referential_transparency_%28computer_science%29>, but I don't know how reliable that is.
If you cannot stand the ugliness of half_sqrt_of_min_dist, just use "tmp" (short for 'temporary') instead. That nicely implies nothing about the variable's content (which is good, as, while we know how it's calculated, we don't seem to be sure about its semantic) and at the same time signifies that we use the variable to store a value /once/, so we don't have to re-compute it /several times/ later on. (Which is true, see paragraph above.) > displ_global.setVec(displ.mV[0] * min_dist, > displ.mV[1] * min_dist, > displ.mV[2] * min_dist); > > // equates to: Displ_global = Displ * M_cam_axes_in_global_frame > --- > Yes, the double nested sqrt is rather strange to behold. The inner > fsqrtf came from the definition of vec_dist() and I wanted it to match > that definition to make absolutely sure that this change is nothing > more than a refactor. > > The inner sqrt is to make it the minimum distance. The outer, and > its subsequent division by two, are what are so confusing: what does > this code do? Why does it do it that way? The comment doesn't seem to > shed much more light than can be gained from reading the code; it > doesn't give the reasons behind the functionality. My guess is that the formula was found by trail and error to make it "feel right" when moving objects with a SpaceNavigator. Probably for some arbitrary value of "feel right" that is specific to the developer(s) who did it. It'd have to try it again to know how it actually feels. (While I use my SpaceNavigator for camera movement and avatar navigation a lot, I rarely use it for moving objects.) > Now, as to what fsqrtf is... According to math.h, it is part of a > multiple-level #define chain that results in the following: fsqrtf(x) > = sqrtf(x) = ((F32)sqrt((F64)(x))) > > So aside from the extra type conversions, fsqrtf is practically the > same as sqrt. Does that change anything? Should I just make both > sqrts one or the other? As obj_pos and getOrigin() are both of type LLVector3, not of type LLVector3d, sqrt(min_dist_squared) yield the same result as ((F32)sqrt((F64)(min_dist_squared))) even if we were to change the type of min_dist_squared to F64, I think. > Or should I just leave this mess and move on to making sure the rest > of the patch is done? Yes, I'd say so. > (Which it practically is, we are just quibbling over the last > stragglers as far as I can tell.) Indeed. :-) Cheers, Boroondas
_______________________________________________ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
