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

Cron Stardust kf6kjg at gmail.com
Sat Mar 12 18:59:31 PST 2011



> On March 11, 2011, 3:53 a.m., Boroondas Gupte wrote:
> > indra/newview/llnetmap.cpp, line 337
> > <http://codereview.secondlife.com/r/199/diff/1/?file=1187#file1187line337>
> >
> >     Maybe add a short comment here, that this value is meant to be overwritten in the loop below it.

Good idea!  I'll do so.


> On March 11, 2011, 3:53 a.m., Boroondas Gupte wrote:
> > indra/newview/llpanelpeople.cpp, lines 161-162
> > <http://codereview.secondlife.com/r/199/diff/1/?file=1188#file1188line161>
> >
> >     For clarity, please rename these variables to dist1_squared and dist2_squared, too. Or eliminate them by calling dist_vec_squared right in the return statement:
> >     		return dist_vec_squared(item1_pos, me_pos) < dist_vec_squared(item2_pos, me_pos);
> >     
> >     (A bit long, but still shorter than the lines right above it.)

I think I'll go with the idea of remove the extraneous variables.  I don't think it makes the code that much less readable or understandable, and it leaves less for the compiler to have to optimize away. (Not that that last is a reason that can stand up on its own; I simply use it as spice for a stronger reason...)


> On March 11, 2011, 3:53 a.m., Boroondas Gupte wrote:
> > indra/newview/llselectmgr.cpp, lines 6574-6587
> > <http://codereview.secondlife.com/r/199/diff/1/?file=1189#file1189line6574>
> >
> >     Memory reuse is good, I guess, but having variable names that only describe the variable's content correctly part of the time bothers me.
> 
> Oz Linden wrote:
>     Agree with Boroondas here - the variable used inside the loop should be min_dist_squared, not min_dist.
>     
>     Leave the memory optimization to the compiler.

No problem.  I was just using what I had on hand. :)


> On March 11, 2011, 3:53 a.m., Boroondas Gupte wrote:
> > indra/newview/llselectmgr.cpp, lines 6589-6594
> > <http://codereview.secondlife.com/r/199/diff/1/?file=1189#file1189line6589>
> >
> >     Off course, this variable already changed meaning during execution before your change, so ... meh.
> 
> Oz Linden wrote:
>     but since you're touching it anyway, fix it

QSL: I'm on it! :D


- Cron


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


On March 12, 2011, 6:33 a.m., Cron Stardust wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/199/
> -----------------------------------------------------------
> 
> (Updated March 12, 2011, 6:33 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/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/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/20110313/ff54ca7b/attachment-0001.htm 


More information about the opensource-dev mailing list