[vtk-developers] BUG: vtkImageActor - set of ComputedDisplayExtent variable.

[Dave Partyka]

Hi David Gobbi,

It appears from git blame that you are the author, or at least, the last
person to modify this code. What are your thoughts? Is this a bug or
intended as Karthik described?

The blobs of code are:

 // if the display extent has not been set, then compute one
  int *wExtent = input->GetWholeExtent();
  if (this->DisplayExtent[0] == -1)
    {
    this->ComputedDisplayExtent[0] = wExtent[0];
    this->ComputedDisplayExtent[1] = wExtent[1];
    this->ComputedDisplayExtent[2] = wExtent[2];
    this->ComputedDisplayExtent[3] = wExtent[3];
    this->ComputedDisplayExtent[4] = wExtent[4];
    this->ComputedDisplayExtent[5] = wExtent[4];
    }

and

 // if the display extent has not been set, then compute one
  int *wExtent = this->Input->GetWholeExtent();
  if (this->DisplayExtent[0] == -1)
    {
    this->ComputedDisplayExtent[0] = wExtent[0];
    this->ComputedDisplayExtent[1] = wExtent[1];
    this->ComputedDisplayExtent[2] = wExtent[2];
    this->ComputedDisplayExtent[3] = wExtent[3];
    this->ComputedDisplayExtent[4] = wExtent[4];
    this->ComputedDisplayExtent[5] = wExtent[4];
    }


On Wed, Jul 14, 2010 at 9:54 AM, Karthik Krishnan <
karthik.krishnan at kitware.com> wrote:

> Dave:
>
> I would recommend holding off that commit. I'm not convinced that its a
> bug. Specifically, the fragment of code reads :
>
>
> int vtkImageActor::RenderOpaqueGeometry(vtkViewport* viewport)
> {
> ....
>
>  int *wExtent = input->GetWholeExtent();
>   if (this->DisplayExtent[0] == -1)
>     {
>     this->ComputedDisplayExtent[0] = wExtent[0];
>     this->ComputedDisplayExtent[1] = wExtent[1];
>     this->ComputedDisplayExtent[2] = wExtent[2];
>     this->ComputedDisplayExtent[3] = wExtent[3];
>     this->ComputedDisplayExtent[4] = wExtent[4];
>
>     this->ComputedDisplayExtent[5] = wExtent[4];
>     }
>   input->SetUpdateExtent(this->ComputedDisplayExtent);
>   ...
> }
>
> An ImageActor should have flattened extents along some dimension. It does
> not make sense for the ImageActor to update the WHOLE input. It appears that
> with the fix, "this->ComputedDisplayExtent[5] = wExtent[4];", it will lead
> to updating the entire image.
>
> I think the author of the class simply intended to set the display extent
> to the first axial slice if they haven't been set.
>
> Pavel: I'm also interested in knowing how you uncovered/ concluded that
> this is a bug. We use vtkImageActor quite heavily and haven't had any
> issues; Maybe we haven't covered this "if" block, but still, its good to
> know..
>
> Thanks
> --
> karthik
>
>
> On Wed, Jul 14, 2010 at 7:09 PM, Dave Partyka <dave.partyka at kitware.com>wrote:
>
>> Hi Pavel, I am curious how you discovered this bug. Were you just browsing
>> through the file or do you have code that is affected by this? If so would
>> you mind sharing it? I would like to make a test out of it if possible.
>>
>>
>> On Wed, Jul 14, 2010 at 9:26 AM, Dave Partyka <dave.partyka at kitware.com>wrote:
>>
>>> Those lines are definitely covered. Though it appears the only are used
>>> when the extents haven't been set yet.
>>>
>>>
>>> http://www.cdash.org/CDash/viewCoverageFile.php?buildid=663956&fileid=10980615
>>>
>>>
>>> On Wed, Jul 14, 2010 at 9:18 AM, David Cole <david.cole at kitware.com>wrote:
>>>
>>>> This sort of error looks like it should produce some sort of seriously
>>>> incorrect results.
>>>>
>>>> Do we not have a test that covers this code? Is the coverage for the
>>>> lines in question "0" or do we just not have a *good* test that runs through
>>>> that code...?
>>>>
>>>> I would think vtkImageActor is a heavily used class.
>>>>
>>>>
>>>> ?
>>>>
>>>>
>>>> On Wed, Jul 14, 2010 at 8:51 AM, Dave Partyka <dave.partyka at kitware.com
>>>> > wrote:
>>>>
>>>>> Thanks for the fix! I have just committed it.
>>>>>
>>>>> On Wed, Jul 14, 2010 at 4:19 AM, Pokutnev, Pavel (GE EntSol, SensInsp)
>>>>> <Pavel.Pokutnev at ge.com> wrote:
>>>>>
>>>>>>  Hello vtk-devs,
>>>>>>
>>>>>> it looks like there is a copy & paste bug in vtkImageActor line 250
>>>>>> and 293:
>>>>>>
>>>>>> The line:
>>>>>>   this->ComputedDisplayExtent[5] = wExtent[4];
>>>>>> should be:
>>>>>>   this->ComputedDisplayExtent[5] = wExtent[5];
>>>>>>
>>>>>> Best regards!
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> Powered by www.kitware.com
>>>>>>
>>>>>> Visit other Kitware open-source projects at
>>>>>> http://www.kitware.com/opensource/opensource.html
>>>>>>
>>>>>> Follow this link to subscribe/unsubscribe:
>>>>>> http://www.vtk.org/mailman/listinfo/vtk-developers
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Powered by www.kitware.com
>>>>>
>>>>> Visit other Kitware open-source projects at
>>>>> http://www.kitware.com/opensource/opensource.html
>>>>>
>>>>> Follow this link to subscribe/unsubscribe:
>>>>> http://www.vtk.org/mailman/listinfo/vtk-developers
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>> _______________________________________________
>> Powered by www.kitware.com
>>
>> Visit other Kitware open-source projects at
>> http://www.kitware.com/opensource/opensource.html
>>
>> Follow this link to subscribe/unsubscribe:
>> http://www.vtk.org/mailman/listinfo/vtk-developers
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/vtk-developers/attachments/20100714/e9586701/attachment.html>