Am 13.02.2017 um 17:15 schrieb William F Pokorny:

> OK. The trigger for this segmentation fault seems to be commit: c52c176
> "Implemented basic Wavefront OBJ import. (#90)" where we also made
> changes to:
> 
> source/core/bounding/boundingbox.cpp
> 
> and
> 
> source/core/bounding/boundingbox.h
> 
> The specific trigger is a change in a find_axis() for loop where the
> problem additions have been commented so things work:
> 
>     for(i = first; i < last; i++)
>     {
>         bbox = &(Finite[i]->BBox);
> 
>         if(bbox->lowerLeft[X] < mins[X])
>             mins[X] = bbox->lowerLeft[X];
> 
>         if(bbox->lowerLeft[X] + bbox->size[X] > maxs[X])
>             maxs[X] = bbox->lowerLeft[X]; // + bbox->size[X]; // <--
> 
>         if(bbox->lowerLeft[Y] < mins[Y])
>             mins[Y] = bbox->lowerLeft[Y];
> 
>         if(bbox->lowerLeft[Y] + bbox->size[Y] > maxs[Y])
>             maxs[Y] = bbox->lowerLeft[Y]; // + bbox->size[Y]; // <--
> 
>         if(bbox->lowerLeft[Z] < mins[Z])
>             mins[Z] = bbox->lowerLeft[Z];
> 
>         if(bbox->lowerLeft[Z] + bbox->size[Z] > maxs[Z])
>             maxs[Z] = bbox->lowerLeft[Z]; // + bbox->size[Z]; // <--
>     }

What exactly makes you think these changes have anything to do with the
segfault?

I very much suspect that instead of the changes causing the segfault,
the version prior to the commit just masked it somehow. One such
mechanism could be via simply shifting around code so that it reacts
differently to a highly unstable situation. Another mechanism (and the
one I'd currently put my money on) would be via creating a different
bounding box hierarchy.

If you have a closer look at the original `find_axis()` code, you'll
notice that it is very difficult to make heads or tails of the function
if you presume the implementation to be correct -- especially the lines
in question seem rather puzzling. If on the other hand you discard the
idea that the function is bug-free, and presume that the loop's original
intention was simply to set `mins` and `maxs` to the "bottom-left" and
"top-right" corner, respectively, of a box encompassing all the bounding
boxes iterated over, then things suddenly fall into place perfectly: The
function as a whole is then intended to compute the axis along which
that all-encompassing box extends the most. This also fits nicely with
the calling code.

If you go along with that interpretation, then the changes done in
c52c176 are a no-brainer bugfix; and in examining the calling code, it
must be concluded that although the original code would not have created
any truly broken bounding box hierarchies (in the sense that other code
would have gagged on them), it would in semi-rare cases have created
/different/ bounding box hierarchies than intended (and created by the
new version).


> I see no difference in results, but I do not know the bounding code well
> enough to know if just backing the above additions from c52c176 is safe.

The broken code has been undetected ever since its first implementation
in POV-Ray 2.0, so it sure doesn't cause artifacts. It might have an
impact on performance though.


> The only code therein looking to be tangled with the bounding structure is:
> 
> view.cpp:bool View::CheckCameraHollowObject(const Vector3d& point, const
> BBOX_TREE *node)
> 
> which is in fact the code Dick saw in his fails.

Indeed.

One interpretation would be that there is some other bug in the bounding
mechanism that only surfaces in rare circumstances, which happen to be
met in Dick's scene, but which used to be masked by the old bug in
`find_axis()`.

However, one thing that's seriously odd about this issue is that the
reported crash does /not/ happen during an attempt to access the
bounding hierarchy, but in an attempt to access
`viedData.GetSceneData()` -- which should work (or fail to do so)
totally independent of the bounding hierarchy. This is more indicative
of a totally wild pointer happening to trash `viewData`, or more
specifically the `sceneData` shared pointer therein. In that case, wish
us good luck trying to nail down the culprit, 'cause we'll need lots of it.

> Lastly, I've not been able to get a debug compile of any form to fail
> for me.
> 
> Where do we go from here with this one?

Good question. I haven't been able to trigger an error on Windows
either; neither with frame 4 (as suggested by the ini file provided by
Dick) nor with any frame from 1 to 153. And I don't think it makes much
sense to try the remaining 3000 frames of the animation.

One thing we might do is to go further back in the Git history, but use
the newer `find_axis()` code, in the hope that we might find the commit
that /really/ broke things.

Post a reply to this message