Re: [ggt-devel] Sphere emptiness

SourceForge Headquarters 1320 Columbia Street Suite 310 San Diego, CA 92101 +1 (858) 422-6466

Looks reasonable, clean code, I see no reason not to commit this one...
---
kevin meinert | http://www.subatomiclabs.com


On Thu, Oct 25, 2012 at 5:02 PM, Paul Martz <pm...@sk...> wrote:

> Kevin / Sebastian -- This code change has lots of parts, so I want to make
> the changes incrementally so that you can review the code changes and
> commit them (if they're acceptable) as I go.
>
> Attached please find the first patch. It's as I described below for #1 and
> #2, except I went with "mInitialized" and "isInitialized()" for
> readability. Note that this is the opposite sense of the deprecated
> "mEmpty", so much of the patch is devoted to swapping true and false, and
> adding/removing "!" operators, for the existing Box code.
>
> Let me know if this flies (I expect it well, as it's the most trivial of
> the changes), then I'll proceed with the other mods. Thanks!
>    -Paul
>
>
>
>
> On 10/25/2012 11:30 AM, Kevin Meinert wrote:
>
>> For 3, I would leave out conditional and document function that vector
>> must contain initialized volumes only, for performance to avoid
>> conditional check per element (can make suggestion that user of
>> function manage list to not include unititialized)
>>
>>
>>
>> Sent from my Phone
>> From: Paul Martz
>> Sent: 10/25/2012 11:23 AM
>> To: ggt...@li....**net <ggt...@li...>
>> Subject: Re: [ggt-devel] Sphere emptiness
>> (AABox::isEmpty(), yes, as per Sebastian's email.)
>>
>> Suggestions to make this change work for everyone:
>>
>> 1. Change mEmpty to mUninitialized in Box, and add mUninitialized to
>> Sphere. Add
>> an isInitialized() function to both Sphere and Box, but also keep
>> isEmpty().
>> This give us a better name and also maintains backwards compatibility.
>>
>> 2. For the extendVolume() variants that extend a Sphere by a single Point
>> or a
>> single other volume, add the initialization checks to make Sphere work
>> like box.
>>
>> 3. For performance, add extendVolume() functions that take a std::vector
>> of
>> Points. This would also have the initialization check, but would amortize
>> the
>> cost of the check over all the data. Similar code already exists in the
>> makeVolume() functions in Containment.h.
>>
>> Does this sound good?
>>      -Paul
>>
>>
>>
>> On 10/25/2012 9:27 AM, Kevin Meinert wrote:
>>
>>> If no gmtl uses mEmpty,
>>> Then, I question why include it in gmtl.
>>>
>>> If for bookkeeping for higher level subsystems... Then you could easily
>>> extend that class yourself to include that data member, through
>>> inheritance.
>>>
>>>  there are no checks inside the gmtl functions that use the mEmpty
>>>>>
>>>> property directly
>>>
>>> Didn't Paul say below that extendVolume does use mEmpty?
>>>
>>> Sent from my Phone
>>> From: Sebastian Messerschmidt
>>> Sent: 10/25/2012 1:30 AM
>>> To: ggt...@li....**net <ggt...@li...>
>>> Subject: Re: [ggt-devel] Sphere emptiness
>>> Kevin,
>>>
>>>  Got it.
>>>>
>>>> Perhaps a better term would be mExists, or mPositionless, or mUndefined.
>>>>
>>>> Empty implies volume, which 0 could be mistaken for emptiness.
>>>>
>>> mEmpty was the term that was used in the box, so I'd vote for sticking
>>> to it for the sake of readability.
>>> Of course it doesn't make much sense, as there is no such thing as an
>>> empty volume.
>>>
>>>>
>>>> I also would look at the higher level sub system expanding volumes and
>>>> ask why uninitialized ones are even in the list. Leads to the question
>>>> whether it might be more appropriate to do that bookkeeping outside of
>>>> the math system
>>>>
>>> That is right. But in order to do so, It really helps to have the member
>>> ;-)
>>>
>>>>
>>>> Doing conditional checks tends to slow down processing, I always prefer
>>>> keeping a list of what needs work, and blasting through that list (or
>>>> memory coherent array preferably) without any conditionals. My
>>>> preference would be for math code that blazes (avoiding branches) and
>>>> keep conditionals to the higher level...
>>>>
>>> Looking at the code, there are no checks inside the gmtl functions that
>>> use the mEmpty property directly. So no harm done just adding the member.
>>> I totally agree that runtime checking should be done outside the core
>>> functionality. The only arguable option might be an ASSERT, since
>>> merging an "empty" sphere is not valid operation.
>>>
>>>
>>> cheers
>>> Sebastian
>>>
>>>>
>>>> Sent from my Phone
>>>> From: Paul Martz
>>>> Sent: 10/24/2012 1:31 PM
>>>> To: ggt...@li....**net <ggt...@li...>
>>>> Subject: Re: [ggt-devel] Sphere emptiness
>>>>
>>>>
>>>> On 10/24/2012 10:46 AM, Sebastian Messerschmidt wrote:
>>>>
>>>>> Hello Kevin,
>>>>>
>>>>>> What does it mean to have an empty sphere? Or aabox for that matter.
>>>>>> Is
>>>>>> it same as sphere volume == 0. Or is it a flag to signify something
>>>>>> else (I.e. No objects contained in).
>>>>>>
>>>>>> if vol == 0 then why not set the size to 0... If it is so you can
>>>>>> remember previous size, why distinguish 0 volume, why not and an N
>>>>>> volume switch... Why a switch at all...
>>>>>>
>>>>>> Excuse me for not knowing/remembering the purpose of mEmpty...
>>>>>>
>>>>> Imagine you have BoundingVolume (Doesn't matter if it is a box or
>>>>> sphere) that is default initialized.
>>>>> Now if you use this object to extend an existing volume it will expand
>>>>> it in a wrong way.
>>>>> Since an empty sphere (i.e. radius == 0.0) still has an position it
>>>>> will
>>>>> expand the volume to be extended incorrectly.
>>>>> (For example you have sphere1 with pos(10,0,0) and r = 1 and an empty
>>>>> sphere (pos(0,0,0), radius=0). The extendVolume will produce a sphere
>>>>> of
>>>>> pos(5.5,0,0) radius = 6, which is wrong)
>>>>> The main purposes of the empty flag is to ignore those primitives and
>>>>> to
>>>>> check if it was initialized.
>>>>>
>>>> Good summary. Also, an mEmpty member variable will differentiate
>>>> between a
>>>> sphere that contains nothing (uninitialized), and a sphere that
>>>> contains exactly
>>>> one point. Radius == 0 alone doesn't provide this functionality.
>>>>
>>>> As I mentioned previously, I could use a negative radius to indicate an
>>>> empty
>>>> sphere. But, we could also have AABox use mMax<   mMin to indicate an
>>>> empty box,
>>>> and we don't -- instead we have AABox::mEmpty, and code to respect the
>>>> setting
>>>> of that variable in Containment.h. I think, for consistency, Sphere
>>>> should be
>>>> the same way.
>>>>
>>>> I'll proceed with the code change, if no one objects.
>>>>        -Paul
>>>>
>>>>  Your approach testing the volume might work in this case, but it is
>>>>> surely not a general solution.
>>>>> So I vote for Paul's approach.
>>>>>
>>>>> cheers
>>>>> Sebastian
>>>>>
>>>>> P.S. Hi Paul, nice to see you outside the OSG mailing list ;-)
>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Sent from my Phone
>>>>>> From: Paul Martz
>>>>>> Sent: 10/23/2012 5:15 PM
>>>>>> To: ggt...@li....**net<ggt...@li...>
>>>>>> Subject: [ggt-devel] Sphere emptiness
>>>>>> Hi all -- My project uses both gmtl::Sphere and gmtl::AABox as
>>>>>> bounding volumes.
>>>>>> But I've noticed that Sphere lacks the 'bool mEmpty' member variable
>>>>>> that is
>>>>>> present in AABox. Is there a reason for this? If not, I'd like to add
>>>>>> it.
>>>>>>
>>>>>> I would also modify Containment.h's extendVolume(Sphere&,...) to
>>>>>> respect that
>>>>>> variable (as is already done for extendVolume(AABox&,...) ). Finally,
>>>>>> I'd also
>>>>>> add new extendVolume() functions to extend Sphere around an AABox and
>>>>>> vice versa.
>>>>>>
>>>>>> Just wanted to see if there was a logical reason for the current
>>>>>> design before I
>>>>>> embarked on changing the code. I know that I can emulate the desired
>>>>>> behavior
>>>>>> with a negative radius, but it seems to me it would be desirable to
>>>>>> have this
>>>>>> functionality in gmtl rather than only in my project.
>>>>>>
>>>>>> Thanks,
>>>>>>          -Paul
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> ------------------------------**------------------------------**
>>>>>> ------------------
>>>>>> Everyone hates slow websites. So do we.
>>>>>> Make your web apps faster with AppDynamics
>>>>>> Download AppDynamics Lite for free today:
>>>>>> http://p.sf.net/sfu/appdyn_**sfd2d_oct<http://p.sf.net/sfu/appdyn_sfd2d_oct>
>>>>>> ______________________________**_________________
>>>>>> ggt-devel mailing list
>>>>>> ggt...@li....**net <ggt...@li...>
>>>>>> https://lists.sourceforge.net/**lists/listinfo/ggt-devel<https://lists.sourceforge.net/lists/listinfo/ggt-devel>
>>>>>>
>>>>>> ------------------------------**------------------------------**
>>>>>> ------------------
>>>>>> Everyone hates slow websites. So do we.
>>>>>> Make your web apps faster with AppDynamics
>>>>>> Download AppDynamics Lite for free today:
>>>>>> http://p.sf.net/sfu/appdyn_**sfd2d_oct<http://p.sf.net/sfu/appdyn_sfd2d_oct>
>>>>>> ______________________________**_________________
>>>>>> ggt-devel mailing list
>>>>>> ggt...@li....**net <ggt...@li...>
>>>>>> https://lists.sourceforge.net/**lists/listinfo/ggt-devel<https://lists.sourceforge.net/lists/listinfo/ggt-devel>
>>>>>>
>>>>>
>>>>> ------------------------------**------------------------------**
>>>>> ------------------
>>>>> Everyone hates slow websites. So do we.
>>>>> Make your web apps faster with AppDynamics
>>>>> Download AppDynamics Lite for free today:
>>>>> http://p.sf.net/sfu/appdyn_**sfd2d_oct<http://p.sf.net/sfu/appdyn_sfd2d_oct>
>>>>> ______________________________**_________________
>>>>> ggt-devel mailing list
>>>>> ggt...@li....**net <ggt...@li...>
>>>>> https://lists.sourceforge.net/**lists/listinfo/ggt-devel<https://lists.sourceforge.net/lists/listinfo/ggt-devel>
>>>>>
>>>>>
>>>>>  ------------------------------**------------------------------**
>>>> ------------------
>>>> Everyone hates slow websites. So do we.
>>>> Make your web apps faster with AppDynamics
>>>> Download AppDynamics Lite for free today:
>>>> http://p.sf.net/sfu/appdyn_**sfd2d_oct<http://p.sf.net/sfu/appdyn_sfd2d_oct>
>>>> ______________________________**_________________
>>>> ggt-devel mailing list
>>>> ggt...@li....**net <ggt...@li...>
>>>> https://lists.sourceforge.net/**lists/listinfo/ggt-devel<https://lists.sourceforge.net/lists/listinfo/ggt-devel>
>>>>
>>>> ------------------------------**------------------------------**
>>>> ------------------
>>>> Everyone hates slow websites. So do we.
>>>> Make your web apps faster with AppDynamics
>>>> Download AppDynamics Lite for free today:
>>>> http://p.sf.net/sfu/appdyn_**sfd2d_oct<http://p.sf.net/sfu/appdyn_sfd2d_oct>
>>>> ______________________________**_________________
>>>> ggt-devel mailing list
>>>> ggt...@li....**net <ggt...@li...>
>>>> https://lists.sourceforge.net/**lists/listinfo/ggt-devel<https://lists.sourceforge.net/lists/listinfo/ggt-devel>
>>>>
>>>
>>>
>>> ------------------------------**------------------------------**
>>> ------------------
>>> Everyone hates slow websites. So do we.
>>> Make your web apps faster with AppDynamics
>>> Download AppDynamics Lite for free today:
>>> http://p.sf.net/sfu/appdyn_**sfd2d_oct<http://p.sf.net/sfu/appdyn_sfd2d_oct>
>>> ______________________________**_________________
>>> ggt-devel mailing list
>>> ggt...@li....**net <ggt...@li...>
>>> https://lists.sourceforge.net/**lists/listinfo/ggt-devel<https://lists.sourceforge.net/lists/listinfo/ggt-devel>
>>>
>>> ------------------------------**------------------------------**
>>> ------------------
>>> Everyone hates slow websites. So do we.
>>> Make your web apps faster with AppDynamics
>>> Download AppDynamics Lite for free today:
>>> http://p.sf.net/sfu/appdyn_**sfd2d_oct<http://p.sf.net/sfu/appdyn_sfd2d_oct>
>>> ______________________________**_________________
>>> ggt-devel mailing list
>>> ggt...@li....**net <ggt...@li...>
>>> https://lists.sourceforge.net/**lists/listinfo/ggt-devel<https://lists.sourceforge.net/lists/listinfo/ggt-devel>
>>>
>>>
>>>
>> ------------------------------**------------------------------**
>> ------------------
>> Everyone hates slow websites. So do we.
>> Make your web apps faster with AppDynamics
>> Download AppDynamics Lite for free today:
>> http://p.sf.net/sfu/appdyn_**sfd2d_oct<http://p.sf.net/sfu/appdyn_sfd2d_oct>
>> ______________________________**_________________
>> ggt-devel mailing list
>> ggt...@li....**net <ggt...@li...>
>> https://lists.sourceforge.net/**lists/listinfo/ggt-devel<https://lists.sourceforge.net/lists/listinfo/ggt-devel>
>>
>>
>>