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

[Karthik Krishnan]

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/5120ecf1/attachment.html>