[opensource-dev] Review Request: Squared all dist_vec() based comparisons and other dist_vec() operations where sensible.

[Ricky]

And since that failed too, I'll try the tried-and-true email list! Ha!

In llselectmgr.cpp (around lines 6586-6595) we've been beating our heads
against some rather... interesting... code.  I had tried adding
a variable to make things clearer, but I now suggest to jump sideways, and
try another route.  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;
 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.

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?

Or should I just leave this mess and move on to making sure the rest of the
patch is done? (Which it practically is, we are just quibbling over the last
stragglers as far as I can tell.)

Ricky
Cron Stardust

On Mon, Apr 4, 2011 at 4:48 PM, Cron Stardust <kf6kjg at gmail.com> wrote:

>    This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/199/
>
> Huh.. long winded speech became dust on pressing publish...  Here goes again:
>
>
> - Cron
>
> On April 4th, 2011, 10:34 a.m., Cron Stardust wrote:
>   Review request for Viewer.
> By Cron Stardust.
>
> *Updated April 4, 2011, 10:34 a.m.*
> Description
>
> I looking at the code, trying to find out where/how to add a new feature, when I tripped across one of these and it lit my mental warning bells off.  Vector distance comparisons should, IMHO, always be done squared.  So I did some greppin, manual analysis, and some careful modification, and here's the result.
>
>   Testing
>
> Compiled a test viewer and used it, undertaking some of my normal activities.  Results felt good, but are currently anecdotal.  Any suggestions on how to properly measure this (or even some actual measurement from those already instrumented to measure these things,) would be great!
>
>   *Bugs: * VWR-25126 <http://jira.secondlife.com/browse/VWR-25126>
> Diffs
>
>    - doc/contributions.txt (344d4c6d7d7e)
>    - indra/llcharacter/llbvhloader.cpp (344d4c6d7d7e)
>    - indra/llcommon/indra_constants.h (344d4c6d7d7e)
>    - indra/llmath/tests/llbbox_test.cpp (344d4c6d7d7e)
>    - indra/newview/llagent.cpp (344d4c6d7d7e)
>    - indra/newview/llfloaterchat.cpp (344d4c6d7d7e)
>    - indra/newview/llhudeffectlookat.cpp (344d4c6d7d7e)
>    - indra/newview/llhudeffectpointat.cpp (344d4c6d7d7e)
>    - indra/newview/llmaniprotate.cpp (344d4c6d7d7e)
>    - indra/newview/llmanipscale.cpp (344d4c6d7d7e)
>    - indra/newview/llnetmap.cpp (344d4c6d7d7e)
>    - indra/newview/llpanelpeople.cpp (344d4c6d7d7e)
>    - indra/newview/llselectmgr.cpp (344d4c6d7d7e)
>    - indra/newview/llspeakers.cpp (344d4c6d7d7e)
>    - indra/newview/llviewerchat.cpp (344d4c6d7d7e)
>    - indra/newview/llviewermessage.cpp (344d4c6d7d7e)
>    - indra/newview/llviewerparceloverlay.cpp (344d4c6d7d7e)
>    - indra/newview/llvoicevivox.cpp (344d4c6d7d7e)
>    - indra/newview/llworld.cpp (344d4c6d7d7e)
>
> View Diff <http://codereview.secondlife.com/r/199/diff/>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110404/826c248e/attachment.htm