Visualization - Dump improvement for V3d, Graphic3d, Aspect

Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Thu Feb 6 18:17:35 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect

Branch [archived branch] has been created by Commenter 3.

[revision removed]


No new revisions were added by this update.

Branch [archived branch] has been updated by Commenter 3.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Thu Feb 6 18:17:35 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect
    
    (cherry picked from [revision removed])

Branch [archived branch] has been updated by Commenter 3.

[revision removed]


Detailed log of new commits:

Author: Commenter 3
Date: Mon Feb 10 11:05:24 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect
    
    Implements connection of VInspector to DumpJson of V3d_Viewer. It's placed in AIS_InteractiveContext->Properties->V3d_Viewer

Branch [archived branch] has been updated by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Wed Feb 12 14:10:38 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect
    
    #DumpJson for V3d_Viewer

Branch [archived branch] has been created by Commenter 3.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Thu Feb 6 18:17:35 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect

Branch [archived branch] has been updated by Commenter 3.

[revision removed]


Detailed log of new commits:

Author: Commenter 3
Date: Thu Feb 13 07:31:44 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect
    
    # warnings, virtual corrections

Branch [archived branch] has been created by Commenter 3.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Thu Feb 6 18:17:35 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect

Branch [archived branch] has been created by Commenter 3.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Thu Feb 6 18:17:35 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect

Branch [archived branch] has been created by Commenter 3.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Thu Feb 6 18:17:35 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect

Branch [archived branch] has been created by Commenter 3.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Thu Feb 6 18:17:35 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect
    
    (cherry picked from [revision removed])
    
    # Conflicts:
    # src/Graphic3d/Graphic3d_CView.cxx
    # src/Graphic3d/Graphic3d_WorldViewProjState.hxx
    # src/SelectMgr/SelectMgr_TriangularFrustumSet.cxx
    # src/SelectMgr/SelectMgr_TriangularFrustumSet.hxx
    (cherry picked from [revision removed])
    (cherry picked from [revision removed])

Branch [archived branch] has been updated by Commenter 3.

[revision removed]


Detailed log of new commits:

Author: Commenter 3
Date: Thu Jun 25 17:54:39 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect
    
    correct warning on Debian. Removing not-used field.

Branch [archived branch] has been created by Commenter 3.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Thu Feb 6 18:17:35 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect
    
    remove not-used myCamStartOpBnd in V3d_View.

Branch [archived branch] has been deleted by Commenter 3.

[revision removed]

Branch [archived branch] has been updated by Commenter 3.

[revision removed]


Detailed log of new commits:

Author: Commenter 3
Date: Thu Jun 25 18:18:24 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect
    
    out of commit modification

Branch [archived branch] has been created by Commenter 3.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Thu Feb 6 18:17:35 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect
    
    remove not used myCamStartOpBnd field of V3d_View.

Dear Kirill,
could you please review the proposed patch.

OCCT branch: [archived branch],
Products branch: [archived branch]
Jenkins job: http://jenkins-test-12.nnov.opencascade.com/view/CR31354-master-NDS/

Thank you in advance, Natalia

--- a/src/Aspect/Aspect_Background.hxx
+++ b/src/Aspect/Aspect_Background.hxx
+  //! Dumps the content of me into the stream
+  Standard_EXPORT virtual void DumpJson (Standard_OStream& theOStream, Standard_Integer theDepth = -1) const;
...
--- a/src/Aspect/Aspect_GradientBackground.hxx
+++ b/src/Aspect/Aspect_GradientBackground.hxx
+  //! Dumps the content of me into the stream
+  Standard_EXPORT virtual void DumpJson (Standard_OStream& theOStream, Standard_Integer theDepth = -1) const Standard_OVERRIDE;

Unexpected virtual in non-virtual classes.

--- a/src/Aspect/Aspect_Window.hxx
+++ b/src/Aspect/Aspect_Window.hxx
+  //! Dumps the content of me into the stream
+  Standard_EXPORT void DumpJson (Standard_OStream& theOStream, Standard_Integer theDepth = -1) const;

As it is virtual class, it sounds reasonable declaring this method also virtual.

--- a/src/Graphic3d/Graphic3d_Aspects.hxx
+++ b/src/Graphic3d/Graphic3d_Aspects.hxx
-  Standard_EXPORT void DumpJson (Standard_OStream& theOStream, Standard_Integer theDepth = -1) const;
+  Standard_EXPORT virtual void DumpJson (Standard_OStream& theOStream, Standard_Integer theDepth = -1) const;

Graphic3d_Aspects is not a real classes hierarchy - it has no virtual methods (save destructor nested from Standard_Transient)
and has only "dummy" sub-classes (basically for preserving an old API).
It doesn't look reasonable declaring DumpJson virtual here.

+void Graphic3d_CLight::DumpJson (Standard_OStream& theOStream, Standard_Integer theDepth) const
...
+  OCCT_DUMP_FIELD_VALUES_DUMPED (theOStream, theDepth, &myParams)

In this way Graphic3d_CLight::DumpJson() reflects debugger rather than dumper.
Providing named field like ConstAttenuation()/LinearAttenuation()/Angle()/Concentration() depending on light source type would make more sense.

+  OCCT_DUMP_FIELD_VALUES_DUMPED (theOStream, theDepth, myCamera.get())

myBaseXRCamera/myPosedXRCamera/myUnitFactor are missing.

+  OCCT_DUMP_FIELD_VALUE_NUMERICAL (theOStream, myFOVy)
+  OCCT_DUMP_FIELD_VALUE_NUMERICAL (theOStream, myFOVyTan)

myFOVyTan reflects myFOVy (as Tan(DTR_HALF * myFOVy)), doesn't look helpful to be included into dump.

+    //! Dumps the content of me into the stream
+    Standard_EXPORT void DumpJson (Standard_OStream& theOStream, Standard_Integer theDepth = -1) const
+    {
+      OCCT_DUMP_FIELD_VALUES_DUMPED (theOStream, theDepth, &Orientation)
+      OCCT_DUMP_FIELD_VALUES_DUMPED (theOStream, theDepth, &MProjection)
+      OCCT_DUMP_FIELD_VALUES_DUMPED (theOStream, theDepth, &LProjection)
+      OCCT_DUMP_FIELD_VALUES_DUMPED (theOStream, theDepth, &RProjection)
+  
+      OCCT_DUMP_FIELD_VALUE_NUMERICAL (theOStream, myIsOrientationValid)
+      OCCT_DUMP_FIELD_VALUE_NUMERICAL (theOStream, myIsProjectionValid)
+    }

Either should be moved to .cxx or Standard_EXPORT removed.
There is no point dumping matrices in case if !myIsOrientationValid and !myIsProjectionValid.

+    Handle(Graphic3d_Layer) aLayer = anIter.Value();

const Handle(Graphic3d_Layer)&

+  for (NCollection_List<Handle(Graphic3d_Layer)>::Iterator anIter (myLayers); anIter.More(); anIter.Next())
+  {
+    Handle(Graphic3d_Layer) aLayer = anIter.Value();
...
+  for (NCollection_DataMap<Graphic3d_ZLayerId, Handle(Graphic3d_Layer)>::Iterator anIter (myLayerIds); anIter.More(); anIter.Next())
+  {
+    Handle(Graphic3d_Layer) aLayerId = anIter.Value();

It doesn't make sense dumping both List and DataMap - the list will be enough.

+
+
+#include <Graphic3d_RenderingParams.hxx>

Unexpected duplex of empty lines.

+  for (NCollection_IndexedMap<Graphic3d_Structure*>::Iterator anIter (myAncestors); anIter.More(); anIter.Next())
+  {
+    Graphic3d_Structure* anAncestor = anIter.Value();
+    OCCT_DUMP_FIELD_VALUES_DUMPED (theOStream, theDepth, anAncestor)
+  }
+
+  for (NCollection_IndexedMap<Graphic3d_Structure*>::Iterator anIter (myDescendants); anIter.More(); anIter.Next())
+  {
+    Graphic3d_Structure* aDescendant = anIter.Value();
+    OCCT_DUMP_FIELD_VALUES_DUMPED (theOStream, theDepth, aDescendant)
+  }

Wouldn't this cause a recursion?

+    Handle(Graphic3d_Structure) aDisplayedStructure = anIter.Value();
...
+    Handle(Graphic3d_Structure) aHighlightedStructure = anIter.Value();
...
+    Handle(Graphic3d_ViewAffinity) aRegisteredObject = anIter.Value();

const Handle()&

+  OCCT_DUMP_FIELD_VALUE_NUMERICAL (theOStream, myDeviceLostFlag)
+
+}

Unexpected empty line.

 
+  void DumpJson (Standard_OStream& theOStream, Standard_Integer) const
+  {
</pre
Missing description.

--- a/src/NCollection/NCollection_Vec2.hxx
+++ b/src/NCollection/NCollection_Vec2.hxx
+#include <Standard_Dump.hxx>


Please remove now redundant "#include <Standard_Dump.hxx>" from NCollection_Vec3.hxx and NCollection_Vec4.hxx



+template <int N>
+void SelectMgr_Frustum<N>::DumpJson (Standard_OStream& theOStream, Standard_Integer theDepth) const
+{
+  OCCT_DUMP_TRANSIENT_CLASS_BEGIN (theOStream)


As SelectMgr_Frustum is template, get_type_name() from OCCT_DUMP_TRANSIENT_CLASS_BEGIN will return "SelectMgr_BaseFrustum" instead of "SelectMgr_Frustum<N>" - is it expected?

Why OCCT_DUMP_BASE_CLASS is not called?



+    gp_Vec anEdgeDir = myEdgeDirs[anIndex];


const gp_Vec&



+void SelectMgr_RectangularFrustum::DumpJson (Standard_OStream& theOStream, Standard_Integer theDepth) const
+{
+  OCCT_DUMP_CLASS_BEGIN (theOStream, SelectMgr_RectangularFrustum)
+  OCCT_DUMP_BASE_CLASS (theOStream, theDepth, SelectMgr_Frustum)


SelectMgr_Frustum is not exactly base class of SelectMgr_RectangularFrustum considering SelectMgr_Frustum<4> transient class.



--- a/src/SelectMgr/SelectMgr_TriangularFrustum.cxx
+++ b/src/SelectMgr/SelectMgr_TriangularFrustum.cxx
+}
\ No newline at end of file


Missing empty line at end of file



+    Handle(Prs3d_ShadingAspect) anArrowShadinAspect = myArrowShadingAspects[anIter];
...
+    Handle(Prs3d_LineAspect) anArrowLineAspect = myArrowLineAspects[anIter];
...
+    Handle(Graphic3d_CLight) anActiveLight = anIterator.Value();
...
+    Handle(V3d_View) aDefinedView = anIter.Value();
...
+    Handle(V3d_View) anActiveView = anIter.Value();
...
+    Handle(Graphic3d_CLight) aDefinedLight = anIter.Value();
...
+    Handle(Graphic3d_CLight) anActiveLight = anIter.Value();


const Handle()&



	

Aspect_Background.hxx, Aspect_GradientBackground.hxx
...
Unexpected virtual in non-virtual classes.

We did DumpJson virtual in Aspect_Background to redefine it in Aspect_GradientBackground.

Ok, virtual will be removed for Aspect_Background/Aspect_GradientBackground.

Branch [archived branch] has been updated by Commenter 3.

[revision removed]


Detailed log of new commits:

Author: Commenter 3
Date: Fri Jun 26 06:37:29 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect
    
    remarks correction

Branch [archived branch] has been created by Commenter 3.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Thu Feb 6 18:17:35 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect
    
    remove not used myCamStartOpBnd field of V3d_View.

+ for (NCollection_IndexedMap<Graphic3d_Structure*>::Iterator anIter (myDescendants); anIter.More();
anIter.Next())
+ {
+ Graphic3d_Structure* aDescendant = anIter.Value();
+ OCCT_DUMP_FIELD_VALUES_DUMPED (theOStream, theDepth, aDescendant)
+ }

Wouldn't this cause a recursion?
>> Agree, it's corrected to dump only pointer now.

SelectMgr_Frustum<N> ...
...is it expected?
>> yes, intentionally and it's enough for now. Either it leads to recursion.

Others are corrected. Please, check once again.

Jenkins job is: http://jenkins-test-12.nnov.opencascade.com/view/CR31354-master-NDS/

Thank you in advance, Natalia

+  if (sqrt(myDirection.x() * myDirection.x() + myDirection.y() * myDirection.y() + myDirection.z() * myDirection.z()) > gp::Resolution())
+  {
+    gp_Dir aDirection (myDirection.x(), myDirection.y(), myDirection.z());
+    OCCT_DUMP_FIELD_VALUES_DUMPED (theOStream, theDepth, &aDirection)
+  }
+  OCCT_DUMP_FIELD_VALUE_NUMERICAL (theOStream, Range())
+
+  OCCT_DUMP_FIELD_VALUE_NUMERICAL (theOStream, ConstAttenuation())
+  OCCT_DUMP_FIELD_VALUE_NUMERICAL (theOStream, LinearAttenuation())
+  OCCT_DUMP_FIELD_VALUE_NUMERICAL (theOStream, Angle())
+  OCCT_DUMP_FIELD_VALUE_NUMERICAL (theOStream, Concentration())

Please take a look onto documentation blocks / assertions in Setters code:

if (myType == Graphic3d_TOLS_SPOT || myType == Graphic3d_TOLS_POSITIONAL)
{
  OCCT_DUMP_FIELD_VALUES_DUMPED (theOStream, theDepth, &myPosition)
}
if (myType == Graphic3d_TOLS_SPOT || myType == Graphic3d_TOLS_DIRECTIONAL)
{
  const Graphic3d_Vec3 aDir = myDirection.xyz();
  OCCT_DUMP_FIELD_VALUES_DUMPED (theOStream, theDepth, &aDir)
}
if (myType == Graphic3d_TOLS_POSITIONAL || myType == Graphic3d_TOLS_SPOT)
{
  OCCT_DUMP_FIELD_VALUE_NUMERICAL (theOStream, ConstAttenuation())
  OCCT_DUMP_FIELD_VALUE_NUMERICAL (theOStream, LinearAttenuation())
}
if (myType == Graphic3d_TOLS_SPOT)
{
  OCCT_DUMP_FIELD_VALUE_NUMERICAL (theOStream, Angle())
  OCCT_DUMP_FIELD_VALUE_NUMERICAL (theOStream, Concentration())
}
if (myType == Graphic3d_TOLS_POSITIONAL || myType == Graphic3d_TOLS_SPOT)
{
  OCCT_DUMP_FIELD_VALUE_NUMERICAL (theOStream, Range())
}

Ok, I saw the redefined light classes, then it's better to provide DumpJson in children for used fields only. I plan though to remove it in parent for now.

But, don't you think that storing not-used fields in a base class is not a good point? We spent more memory that required for each light instance, do we?

Also, it's not very nice to have a crash by calling Direction() of a light. Why crash, it's just a getter. Method with boolean as a result might be more friendly.

I doubt about if (sqrt(myDirection...) checking. What do you reccomend to use instead of it? I tried to use gce_MakeDir, but it's not correct here, leads to linkage error.

Thank you for help

> Ok, I saw the redefined light classes, then it's better to provide DumpJson in children for used fields only.
V3d_*Light classes exist for preserving compatibility with old API.
Although they can be useful for reducing visibility of light properties, there is no much use handling DumpJson() specifically for each sub-class, like in case of Graphic3d_Aspects.

> But, don't you think that storing not-used fields in a base class is not a good point?
> We spent more memory that required for each light instance, do we?
Light source properties are tightly packed - for efficient transferring onto GLSL level. This actually caused confusion in the first version of the patch. There is no much memory can be spared by splitting properties per light source type.

> I doubt about if (sqrt(myDirection...) checking.
As has been quoted previous in post - I propose checking for a type, for which direction is always defined `(myType == Graphic3d_TOLS_SPOT || myType == Graphic3d_TOLS_DIRECTIONAL)`.

get it, thanks

May we have gp_Dir instead of Graphic3d_Vec3 in your proposal?

In Inspector, we'll create a default direction presentation based on parsing gp_Dir instance(#0031326) and will have a preview in inspector view(short line now).
As the parameter is a direction of light, it sounds correct, isn't it?

Graphic3d_Vec3 might be not direction, so it isn't planned to be converted in some preview presentation. So, may we not use it here, and use gp_Dir?

> May we have gp_Dir instead of Graphic3d_Vec3 in your proposal?
I have no objection - use Direction() method in this case construction gp_Dir.

Branch [archived branch] has been deleted by Commenter 3.

[revision removed]

Branch [archived branch] has been created by Commenter 3.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Thu Feb 6 18:17:35 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect
    
    - dump for classes in visu packages
    - remove not used myCamStartOpBnd field of V3d_View.
    - extend Inspector with item for V3d_Viewer and SelectMgr_ViewerSelector3d

Please, review

Branch [archived branch] has been deleted by Commenter 3.

[revision removed]

Branch [archived branch] has been created by Commenter 3.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Thu Feb 6 18:17:35 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect
    
    - dump for classes in visu packages
    - remove not used myCamStartOpBnd field of V3d_View.
    - extend Inspector with item for V3d_Viewer and SelectMgr_ViewerSelector3d

Please raise the patch in OCCT branch CR31354_8.

Problem of compilation on CentOS64-64

http://jenkins-test-11.nnov.opencascade.com:8080/view/WEEK-26_WEEK-26/view/OCCT%20compile/job/WEEK-26_WEEK-26-OCCT-CentOS64-64-opt-compile/1/parsed_console/

Branch [archived branch] has been updated by Commenter 3.

[revision removed]


Detailed log of new commits:

Author: Commenter 3
Date: Sun Jul 5 12:03:33 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect
    
    - do not dump light in view as it's dumped in the viewer

Branch [archived branch] has been created by Commenter 3.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Thu Feb 6 18:17:35 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect
    
    - do not dump light in view as it's dumped in the viewer

Branch [archived branch] has been created by Commenter 3.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Thu Feb 6 18:17:35 2020 +0300

    0031354: Visualization - Dump improvement for V3d, Graphic3d, Aspect
    
    - do not dump light in view as it's dumped in the viewer

Dear Igor,

this branch is re-based on current master:
OCCT: CR31354_10.
OCCT Products: NO.

Could you please restart tests for it on jenkins-test-11.

Thank you a lot, Natalia

Combination -
OCCT branch : [archived branch]
master SHA - [revision removed]
[revision removed]
Products branch : [archived branch] SHA - [revision removed]
was compiled on Linux, MacOS and Windows platforms and tested in optimize mode.

Number of compiler warnings:
No new/fixed warnings

Regressions/Differences/Improvements:
No regressions/differences

CPU differences:
Debian80-64:
OCCT
Total CPU difference: 17319.18000000012 / 17284.370000000214 [+0.20%]
Products
Total CPU difference: 12059.670000000087 / 12007.590000000067 [+0.43%]
Windows-64-VC14:
OCCT
Total CPU difference: 18776.390625 / 18770.796875 [+0.03%]
Products
Total CPU difference: 13251.640625 / 13222.90625 [+0.22%]


Image differences :
No differences that require special attention

Memory differences :
No differences that require special attention

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]