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

[Cron Stardust]

> On April 4, 2011, 2:33 p.m., Boroondas Gupte wrote:
> > indra/newview/llselectmgr.cpp, lines 6591-6592
> > <http://codereview.secondlife.com/r/199/diff/1-2/?file=1189#file1189line6591>
> >
> >     This comment only really makes sense to those aware of this change. Those reading the code later won't easily be able to make any sense of it.
> >     
> >     As a rule of thumb, in-code comments should relate to the resulting code and commit messages to the change. If you feel you have to deviate from that, make it explicit. E.g. here, we could write:
> >     
> >     		// Replaces a call to dist_vec(), which uses fsqrtf. Thus that's what we use here, too.
> >     		F32 min_dist = fsqrtf(min_dist_squared);

The comment is a warning to those who might notice that a few lines further down a different sqrt function is used.  Your edition is clearer though.


> On April 4, 2011, 2:33 p.m., Boroondas Gupte wrote:
> > indra/newview/llselectmgr.cpp, lines 6595-6597
> > <http://codereview.secondlife.com/r/199/diff/2-3/?file=1252#file1252line6595>
> >
> >     While we're touching this code anyway, put spaces around (i.e. on both sides of) the binary * operators (multiplication). That makes it easier to visually distinguish them from unary * operators (dereference).

How did I miss doing that? :P


> On April 4, 2011, 2:33 p.m., Boroondas Gupte wrote:
> > indra/newview/llselectmgr.cpp, lines 6594-6597
> > <http://codereview.secondlife.com/r/199/diff/2-3/?file=1252#file1252line6594>
> >
> >     Even if we decided that it is out of scope to decide what "factor" might have meant here, we can avoid having to place a FIXME comment here by just using a variable name that avoids the term.
> >     
> >     half_sqrt_of_min_dist is admittedly an ugly name and doesn't tell the reader why we would calculate it, but it is at least truthful and not misleading, so I'd really go for that for now.

ugh.. still bugs me.  I agree that the FIXME is obnoxious, and your variable name (as ugly as I agree it is,) is only 4 characters longer.  Lemme try an idea in a parallel review of my own, let me know what you think.


- Cron


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/199/#review544
-----------------------------------------------------------


On April 4, 2011, 10:34 a.m., Cron Stardust wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/199/
> -----------------------------------------------------------
> 
> (Updated April 4, 2011, 10:34 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> -------
> 
> 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.
> 
> 
> This addresses bug 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 
> 
> Diff: http://codereview.secondlife.com/r/199/diff
> 
> 
> 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!
> 
> 
> Thanks,
> 
> Cron
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110404/5bb265cd/attachment-0001.htm