tdf#118498 Correct CustomShape 3D Visualization

Change-Id: Ic312190616044f37fd92464ad605c6e0cdd5cc4d
Reviewed-on: https://gerrit.libreoffice.org/57547
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@cib.de>
(cherry picked from commit ee71c3def71508d1fc3e110659c7ed7aa0ba2238)
Reviewed-on: https://gerrit.libreoffice.org/61207
Reviewed-by: Thorsten Behrens <Thorsten.Behrens@CIB.de>

2 files changed, 29 insertions, 14 deletions

diff --git a/svx/source/sdr/contact/viewcontactofsdrobjcustomshape.cxx b/svx/source/sdr/contact/viewcontactofsdrobjcustomshape.cxx
index 8cabe8d261bf..10089a8b0dd0 100644
--- a/svx/source/sdr/contact/viewcontactofsdrobjcustomshape.cxx
+++ b/svx/source/sdr/contact/viewcontactofsdrobjcustomshape.cxx
@@ -125,22 +125,29 @@ namespace sdr
                 // Hack for calc, transform position of object according
                 // to current zoom so as objects relative position to grid
                 // appears stable
+                // TTTT: Need to check what *exactly* this is doing - in it's current
+                // form it's indeed pretty much a 'hack' as mentioned above and massively
+                // in the way for future changes...
                 const_cast< SdrObject* >( pSdrObjRepresentation )->SetGridOffset( aGridOff );
-                SdrObjListIter aIterator(*pSdrObjRepresentation);
 
-                while(aIterator.IsMore())
-                {
-                    SdrObject& rCandidate = *aIterator.Next();
-                    // apply offset to each part
-                    rCandidate.SetGridOffset( aGridOff );
-                    if(!b3DShape && dynamic_cast< E3dObject* >(&rCandidate))
-                    {
-                        b3DShape = true;
-                    }
-
-                    const drawinglayer::primitive2d::Primitive2DContainer xNew(rCandidate.GetViewContact().getViewIndependentPrimitive2DContainer());
-                    xGroup.insert(xGroup.end(), xNew.begin(), xNew.end());
-                }
+                // tdf#118498 The processing of SdrObjListIter for SdrIterMode::DeepNoGroups
+                // did change for 3D-Objects, it now correctly enters and iterates the
+                // SdrObjects in the E3dScene (same as for SdrObjGroup). This is more correct
+                // as the old version which just checked for dynamic_cast<const SdrObjGroup*>
+                // and *only* entered these, ignoring E3dScene as grouping-object.
+                // But how to fix that? Taking back the SdrObjListIter change would be easy, but
+                // not correct. After checking ViewContactOfE3dScene and ViewContactOfGroup
+                // I see that both traverse their children by themselves (on VC-Level,
+                // see createViewIndependentPrimitive2DSequence implementations and usage of
+                // GetObjectCount()). Thus in principle iterating here (esp. 'deep') seems to
+                // be wrong anyways, it might have even created wrong and double geometries
+                // (only with complex CustomShapes with multiple representation SdrObects and
+                // only visible when transparency involved, but runtime-expensive).
+                // Thus: Just do not iterate, will check behaviour deeply.
+                b3DShape = (nullptr != dynamic_cast< const E3dObject* >(pSdrObjRepresentation));
+                const drawinglayer::primitive2d::Primitive2DContainer xNew(
+                    pSdrObjRepresentation->GetViewContact().getViewIndependentPrimitive2DContainer());
+                xGroup.insert(xGroup.end(), xNew.begin(), xNew.end());
             }
 
             if(bHasText || !xGroup.empty())
diff --git a/svx/source/svdraw/svditer.cxx b/svx/source/svdraw/svditer.cxx
index 2e8b8a8e1035..45134a7fb86e 100644
--- a/svx/source/svdraw/svditer.cxx
+++ b/svx/source/svdraw/svditer.cxx
@@ -123,6 +123,14 @@ void SdrObjListIter::ImpProcessMarkList(const SdrMarkList& rMarkList, SdrIterMod
 
 void SdrObjListIter::ImpProcessObj(const SdrObject& rSdrObject, SdrIterMode eMode)
 {
+    // TTTT: Note: The behaviour has changed here, it will now deep-iterate
+    // for SdrObjGroup and E3dScene. Old version only deep-dived for SdrObjGroup,
+    // E3dScene was just added flat. This is now more correct, but potentially
+    // there will exist code in the 3D area that *self-iterates* with local
+    // functions/methods due to this iterator was not doing the expected thing.
+    // These will be difficult to find, but in most cases should do no harm,
+    // but cost runtime. Will need to have an eye on this aspect on continued
+    // changes...
     const SdrObjList* pChildren(rSdrObject.getChildrenOfSdrObject());
     const bool bIsGroup(nullptr != pChildren);