Data Exchange - BinXCAF should preserve length unit information

Branch [archived branch] has been created by Participant.

[revision removed]


Detailed log of new commits:

Author: skl
Date: Tue Nov 10 07:52:30 2020 +0300

    0031382: Data Exchange - BinXCAF should preserve length unit information
    
    Add LengthUnit info to XCAF document.

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Result of tests:
http://vm-jenkins-test-12.nnov.opencascade.com:8080/view/CR31382-master/view/COMPARE/

It would be helpful to add to the LengthUnitTool a method that returned scale ratio between two units.
Use case:

  XCAFDoc_LengthUnitTool::UnitValue aUnit = aLUTool->GetLengthUnit();
  double aScale = XCAFDoc_LengthUnitTool::GetScale (XCAFDoc_LengthUnitTool::UV_METER, aUnit);

GetScale (XCAFDoc_LengthUnitTool::UV_METER, XCAFDoc_LengthUnitTool::UV_MILLIMETER) is expected to return 1000.

Sergey, you can make the code compact (and less error prone) by separating the code that converts a string to the unit enum into a dedicated function in XDEDRAW.cxx.

To make patch actually useful, it is desired to:
1. Update existing XCAF document (e.g. STEP, IGES, glTF, JT) filling/creation tools to set up units information, when it is known.
2. Analyze how to convert XCAF document from one unit to another (while reading or by other means). This might be really complicated considering reading of TopoDS_Shape / PMI information / etc., but feasible.

This is not necessary to be put into this patch, but before processing with integration of this step we should analyze how to go further (and potentially alter this patch to suite better further needs).

What is missing in this fix is storing units in standard saving procedure. Can we use the current value of the parameter xstep.cascade.unit to store the unit in the saved file? If some unit-aware application does not use this parameter it will set the units in the document explicitly, and in this case the standard procedure must not override the value of unit set in the document.

To get conversion from one unit to other you may use method UnitsMethods::GetLengthFactorValue

I don't know, may be it is worth synchronize somehow enumeration with integer values used in this method. May be we could use the same enum in both UnitsMethods and XCAFDoc_lengthUnitTool?

Also in XCAFDoc_LengthUnitTool::SetLengthUnit you may remove everything except the last line, because TDataStd_Integer::Set checks the attribute already exists and changes the value. You don't need to forget the attribute just to change it.

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Result of tests:
http://vm-jenkins-test-12.nnov.opencascade.com:8080/view/CR31382-master/view/COMPARE/

+  enum LengthUnit {

This should be moved to dedicated enum header following recommendations in OCCT Coding Rules.

+UnitsMethods::LengthUnit lengthUnitByStr(TCollection_AsciiString theStr)

static
const TCollection_AsciiString&

+  if (theStr.IsEqual("MM"))

It is preferred making comparison case-insensitive.

+  di.Add("XCheckLengthUnit",

This command makes no sense for general usage - it could be replaced by Tcl comparison check using XGetLengthUnit command.

+  di.Add("XDumpLengthUnit",

XDump* prefix is unusual for getters in Draw Harness plugins - other getters have XGet* prefix.

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

In order to avoid situation with undefined length unit in the document special protection in the XCAFDoc_ShapeTool is added: before adding of first TopoDS_Shape to document value of length unit is checked and if it is undefined, a value from session (param xstep.cascade.unit) sets into document as length unit.

Results of tests:
http://vm-jenkins-test-12.nnov.opencascade.com:8080/view/CR31382-master/view/COMPARE/

+  case UnitsMethods_LengthUnit::LU_MILLIMETER: return "MILLIMETER";

This enumeration value syntax will raise old compiler warning on weekly Jenkins, please drop it.

Please also rename enumeration values to include full enumeration name, e.g.:
- UnitsMethods_LengthUnit::LU_MILLIMETER -> UnitsMethods_LengthUnit_Millimeter

+XSetLengthUnit D IncH
Why last letter is capital? It causes reader to find a hidden sense of it. Please use the simple word instead.

+ Handle(XCAFDoc_LengthUnitTool) aLUTool = XCAFDoc_LengthUnitTool::Set(Label().Father().FindChild(11, Standard_True));
Use LengthUnitLabel from document tool to access to the label of units.

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Result of tests:
http://vm-jenkins-test-12.nnov.opencascade.com:8080/view/CR31382-master/view/COMPARE/

Please rebase on the latest master.

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

For integration:
occt - CR31382
products - none

--- a/src/TKXCAF/EXTERNLIB
+++ b/src/TKXCAF/EXTERNLIB
@@ -10,3 +10,5 @@ TKLCAF
 TKG3d
 TKCAF
 TKVCAF
+TKXSBase

Putting STEP reader/writer as dependency for XCAF document library doesn't look like an acceptable change.

+UnitsMethods_LengthUnitEnum.hxx

File name should be "UnitsMethods_LengthUnit.hxx" according to OCCT coding rules, not "UnitsMethods_LengthUnitEnum.hxx".

+UnitsMethods_LengthUnit lengthUnitByStr(TCollection_AsciiString theStr)

static

+UnitsMethods_LengthUnit lengthUnitByStr(TCollection_AsciiString theStr)
+{
...
+  else
+    return UnitsMethods_LengthUnit_Undefined;
...

+  TCollection_AsciiString aUnit(argv[2]);
+  aLUTool->SetLengthUnit(lengthUnitByStr(argv[2]));

Command is expected to print user an error in case of wrong input, not silently commit UNDEFINED unit.

+  if (argc < 3) {

Unknown arguments (>3) should be reported as a syntax error as well.

+  if (argc < 3) {
...
+enum UnitsMethods_LengthUnit {

Please follow OCCT coding rules in new code - put { on the next line.

@@ -548,6 +551,17 @@ TDF_Label XCAFDoc_ShapeTool::AddShape (const TopoDS_Shape& theShape,
...
+      aLU = (UnitsMethods_LengthUnit)Interface_Static::IVal("xstep.cascade.unit");

Reading STEP-specific parameter within the base tool is illogical.
In contrary, it is a STEP translator filling in XCAF document should put length unit information (and print a warning if document has already specified non-matching length unit).

+UnitsMethods_LengthUnit XCAFDoc_LengthUnitTool::GetLengthUnit() const
...
+    aVal = (UnitsMethods_LengthUnit)anAttribute->Get();

As there is no dedicated driver, no versioning and no guarantee that UnitsMethods_LengthUnit will not be extended in future, it would be helpful verifying the enumeration range before cast.
By the way, I think it might be more useful storing length unit as a string rather than enum in XCAF document.

+  static Standard_GUID LengthUnitID("4eba571f-de90-4840-8639-eab9b72360e0");

static const

+//! The Enumeration describes possible values for length units
+
+enum UnitsMethods_LengthUnit {

Unexpected empty line between documentation and enum.

+};
+#endif // _XCAFDoc_LengthUnitTool_HeaderFile

Please put an empty line after };

Package UnitsMethods is in toolkit TKXSBase.
Is it reasonable to move this package to another toolkit (for example to TKernel) in order to avoid dependency between TKXCAF and TKXSBase?

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

> Is it reasonable to move this package to another toolkit
> (for example to TKernel) in order to avoid
> dependency between TKXCAF and TKXSBase?
UnitsMethods currently defines not only global "cascade units" property (GetCasCadeLengthUnit()), but also auxiliary geometry conversion methods.
So that while technically it looks reasonable moving GetCasCadeLengthUnit() outside, this cannot be done for the whole UnitsMethods package as it has to be split into parts.

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Branch [archived branch] has been updated by Participant.

[revision removed]


Detailed log of new commits:

Author: skl
Date: Fri Dec 25 16:47:44 2020 +0300

    0031382: Data Exchange - BinXCAF should preserve length unit information
    
    Split package UnitsMethods.

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Branch [archived branch] has been updated by Participant.

[revision removed]


Detailed log of new commits:

Author: skl
Date: Mon Dec 28 13:03:29 2020 +0300

    0031382: Data Exchange - BinXCAF should preserve length unit information
    
    Try to avoid using "xstep.cascade.unit".

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Package UnitsMethods is splitted: geom methods was placed to new package UnitsGeomMethods which is in the toolkit TKXSBase. Package UnitsMethods is placed to toolkit TKernel.
Since for all supported formats value CasCadeLengthUnit is used for set unit of length it is reasonable to avoid using of parameter "xstep.cascade.unit".
New Draw command "setlengthunit" for set length unit is added. This command have to be used istead of command "param xstep.cascade.unit".
All using "xstep.cascade.unit" in the read/write classes is removed.
But for supporting of existed Draw tests and other using in "old" code setting for "xstep.cascade.unit" in class XSAlgo.cxx is kept.

Result of tests:
http://vm-jenkins-test-12.nnov.opencascade.com:8080/view/CR31382-CR31382-skl/view/COMPARE/

The description of the method UnitsMethods::GetLengthUnitByFactorValue is not valid. Please correct it.

The new methods FactRD and FactDR in UnitsMethods have unclear names and no description. Please rename them like FactorRadianDegree and add description.

Why did you remove the virtual method XSAlgo_AlgoContainer::PrepareForTransfer? It can be redefined in some other code.

I propose to replace in the tests usage of "param xstep.cascade.unit" with "setlengthunit". It is in order to remove the usage of obsolete mechanism from OCCT.

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Needed changes is done.
Result of tests:
http://vm-jenkins-test-12.nnov.opencascade.com:8080/view/CR31382-CR31382-skl/view/COMPARE/

Again, the description of the method UnitsMethods::GetLengthUnitByFactorValue is not valid. Please correct it.

In XtControl_ActorRead::Transfer(), why did you commented out the following line? I think we should leave the call of this virtual method.
  //XSAlgo::AlgoContainer()->PrepareForTransfer();

Prepare new ready for integration branches with one commit in both occt and products.

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Needed changes is done.
Result of tests:
http://vm-jenkins-test-12.nnov.opencascade.com:8080/view/CR31382-CR31382-skl/view/COMPARE/

> Add LengthUnit info to XCAF document.
> Split package UnitsMethods.
> Try to avoid using "xstep.cascade.unit".

This comment description is too basic - it is expected to see details on modifications (how UnitsMethods package has been split? which interface is introduced in XCAF for LengthUnit info? what does it mean "try to avoid using xstep.cascade.unit"?).

+  di << "current value of length unit is " << UnitsMethods::DumpLengthUnit(aLU);

This is an overloaded output for the command intended to return variable value - please consider removing redundant text and keep only output of value itself.
So that Tcl script could be able to use "set aUnit [lengthunit]" without necessity to parse text.

--- a/src/TKernel/PACKAGES
+++ b/src/TKernel/PACKAGES
@@ -14,3 +14,4 @@ Units
 UnitsAPI
 NCollection
 Message
+UnitsMethods

Please apply alphabetical order for new entities.

+
+class UnitsGeomMethods 

Please provide description for new class and it's methods.

+  
+protected:
+
+private:
+
+};

No need in copying empty sections.

+  if (fabs(1. - theFactorValue) < aPreci)

fabs() -> Abs()

+enum UnitsMethods_LengthUnit {

Please follow OCCT Coding Style (newline before opening bracket).

src/UnitsMethods/UnitsMethods.hxx
  //! Returns the enumeration corresnoding to the given scale factor
Misprint "corresnoding"

Results of tests:
http://vm-jenkins-test-12.nnov.opencascade.com:8080/view/CR31382-CR31382-skl/view/COMPARE/

I have corrected the spelling of commit message in CR31382_1.

For integration:
occt - CR31382_1
products - CR31382

+static Standard_Integer setLengthUnit(Draw_Interpretor& di, Standard_Integer argc, const char** argv)
+{
+  if (argc < 2)
...
+static Standard_Integer lengthUnit(Draw_Interpretor& di, Standard_Integer 
+{
+  UnitsMethods_LengthUnit aLU = UnitsMethods::GetLengthUnitByFactorValue(UnitsMethods::GetCasCadeLengthUnit());
+  di << UnitsMethods::DumpLengthUnit(aLU);
+  return 0;

This is a weak check - it allows passing extra arguments to commands without user noticing they will be ignored. Please check for equality instead.

+  theCommands.Add("lengthunit", "return value of length unit"
+  theCommands.Add("setlengthunit", "set value of length unit"

Defining two commands for the same thing looks redundant - it will be simpler having a single command "lengthunit" doing both tasks depending on a number of arguments, as many other Draw Harness commands.

--- a/src/Interface/Interface_Static.cxx
+++ b/src/Interface/Interface_Static.cxx
+    if (strcmp(name, "xstep.cascade.unit") == 0)
+    {
+      Standard_Integer aUnit = item->IntegerValue();
+      UnitsMethods::SetCasCadeLengthUnit(aUnit);
+    }

Keeping this hack indefinitely within the basic interface for arbitrary parameters doesn't look like a good idea.
I propose generating a runtime warning (Message::SendWarning()) about deprecation of "xstep.cascade.unit" parameter, so that we will remove this code after a couple of OCCT releases.

+//! Class contains conversion methods for 2d geom objects
+class UnitsGeomMethods
+{

I don't know if these methods have been supposed to be public / have any use for external applications, but if yes - this API change should be mentioned in Upgrade Guide, as user will be unable to figure out where the methods have gone.

Solution elaboration
Elaboration storing of new XCAF attribute ( Units )

Solution elaboration

Solution elaboration & testing

Implementation of a new attribute for storing units

Solution elaboration & testing

Solution elaboration

Solution elaboration

Solution elaboration

Solution elaboration

Solution implementation

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

[revision removed]


Detailed log of new commits:

Author: Commenter 4
Date: Tue Nov 10 07:52:30 2020 +0300

    0031382: Data Exchange - BinXCAF should preserve length unit information
    
    Possibility for adding LengthUnit info to XCAF document using special class TDataStd_LenghtUnit is implemented
    Package UnitsMethods is splited: geom methods was placed to new file GeomConvert_Units which is in the toolkit TKXSBase, internal step scale factors was placed to StepData
    All global units was replace by UnitsAPI
    Since for all supported formats value CasCadeLengthUnit is used for set unit of length using of parameter "xstep.cascade.unit" for set length unit is not needed.
    New Draw command "setlengthunit" for set value of length unit is added. This command have to be used istead of command "param xstep.cascade.unit".
    All using "xstep.cascade.unit" in the read/write classes is removed.
    Initialization of "xstep.cascade.unit" is kept in the method XSAlgo::Init() in order to support old versions.

Solution implementation & testing

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

A remark following our discussion: we do not seem to have a strong need of global parameter defining units of OCCT apart of DE components (which use xstep.cascade,units). These units are defined by the application and can be managed in application-specific way (usially this is not customizable).

At the same time, there is apparently a need to have possibility to convert XCAF document defined in one units, to some other units. This operation shall be performed after loading XCAF document if it was written in units different from those used by the application.

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

[revision removed]


Detailed log of new commits:

Author: Commenter 4
Date: Tue May 25 13:45:45 2021 +0300

    // fix unit initialization ( test version )

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

Solution elaboration

Revert old realization. Development according to the splitting of the task

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

[revision removed]


Detailed log of new commits:

Author: Commenter 4
Date: Tue Nov 10 07:52:30 2020 +0300

    0031382: Data Exchange - BinXCAF should preserve length unit information
    
    Possibility for adding LengthUnit info to XCAF document using special class TDataStd_LenghtUnit is implemented
    Package UnitsMethods is splited: geom methods was placed to new file GeomConvert_Units which is in the toolkit TKXSBase, internal step scale factors was placed to StepData
    Since for all supported formats value CasCadeLengthUnit is used for set unit of length using of parameter "xstep.cascade.unit" for set length unit is not needed.
    New Draw command "XSetlengthunit" for set XDE attribute for in the doc.

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

Solution implementation

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

[revision removed]


Detailed log of new commits:

Author: Commenter 4
Date: Tue Nov 10 07:52:30 2020 +0300

    0031382: Data Exchange - BinXCAF should preserve length unit information
    
    Possibility for adding LengthUnit info to XCAF document using special class TDataStd_LenghtUnit is implemented
    Package UnitsMethods is splited: geom methods was placed to new file GeomConvert_Units which is in the toolkit TKXSBase, internal step scale factors was placed to StepData
    Since for all supported formats value CasCadeLengthUnit is used for set unit of length using of parameter "xstep.cascade.unit" for set length unit is not needed.
    New Draw command "XSetlengthunit" for set XDE attribute for in the doc.
    Upgraded tests\de to check "loop back" algorithms with any unit

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

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

[revision removed]


Detailed log of new commits:

Author: Commenter 4
Date: Tue Nov 10 07:52:30 2020 +0300

    0031382: Data Exchange - BinXCAF should preserve length unit information
    
    Possibility for adding LengthUnit info to XCAF document using special class TDataStd_LenghtUnit is implemented
    Package UnitsMethods is splited: geom methods was placed to new file GeomConvert_Units which is in the toolkit TKXSBase, internal step scale factors was placed to StepData
    Since for all supported formats value CasCadeLengthUnit is used for set unit of length using of parameter "xstep.cascade.unit" for set length unit is not needed.
    New Draw command "XSetlengthunit" for set XDE attribute for in the doc.
    Upgraded tests\de to check "loop back" algorithms with any unit

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

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

[revision removed]


Detailed log of new commits:

Author: Commenter 4
Date: Fri Aug 13 19:05:11 2021 +0300

    // Added Length unit attribute to Doc as new type of an attribute

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

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

[revision removed]


Detailed log of new commits:

Author: Commenter 4
Date: Tue Nov 10 07:52:30 2020 +0300

    0031382: Data Exchange - BinXCAF should preserve length unit information
    
    Possibility for adding LengthUnit info to XCAF document using special class XCAFDoc_LenghtUnit and XCAFDoc_LenghtUnitTool is implemented
    Package UnitsMethods is splited: geom methods was placed to new file GeomConvert_Units which is in the toolkit TKXSBase, internal step scale factors was placed to StepData
    Updated UnitMethods to convert scale factor to different unit types.
    Now, XSAlgo::XSAlgo_AlgoContainer used for update unit info from static interface values.
    New Draw command "XSetlengthunit" and "XGetLengthUnit" for set or get XDE attribute
    Upgraded tests for step, iges, obj, gltf, vrlm formats to check area regressing with used unit
    Upgraded tests\de test cases to use any units in the "loop back" algorithms

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

[revision removed]


Detailed log of new commits:

Author: Commenter 4
Date: Wed Aug 18 21:09:42 2021 +0300

    // remarks have been fixed

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

[revision removed]


Detailed log of new commits:

Author: Commenter 4
Date: Wed Aug 18 22:45:10 2021 +0300

    // fixed problem with StepCafControl_Writer and IGESCAFControl_Writer transferring

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

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

[revision removed]


Detailed log of new commits:

Author: Commenter 4
Date: Tue Nov 10 07:52:30 2020 +0300

    0031382: Data Exchange - BinXCAF should preserve length unit information
    
    Possibility for adding LengthUnit info to XCAF document using special class XCAFDoc_LenghtUnit and XCAFDoc_LenghtUnitTool is implemented
    Package UnitsMethods is splited: geom methods was placed to new file GeomConvert_Units which is in the toolkit TKXSBase, internal step scale factors was placed to StepData
    Updated UnitMethods to convert scale factor to different unit types.
    Now, XSAlgo::XSAlgo_AlgoContainer used for update unit info from static interface values.
    New Draw command "XSetLengthUnit" and "XGetLengthUnit" for set or get XDE attribute
    Upgraded tests for step, iges, obj, gltf, vrlm formats to check area regressing with used unit
    Upgraded tests\de test cases to use any units in the "loop back" algorithms

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

Dear KGV,
please review the CR31382_7

OCCT: CR31382_7
PRODUCTS: NO

All tests are OK, see:
http://jenkins-test-08.nnov.opencascade.com/view/CR31382-master-Commenter 4/view/COMPARE/

Branch [archived branch] has been updated forcibly by Commenter 4.

[revision removed]

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: 17364.550000000334 / 17371.860000000408 [-0.04%]
Products
Total CPU difference: 11522.500000000102 / 11493.740000000085 [+0.25%]
Windows-64-VC14:
OCCT
Total CPU difference: 19242.96875 / 19148.140625 [+0.50%]
Products
Total CPU difference: 12876.5625 / 12805.328125 [+0.56%]


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]