MantisBT - Open CASCADE
View Issue Details
0031382Open CASCADE[OCCT] OCCT:Data Exchangepublic2020-02-18 17:302021-01-13 16:25
[OCCT] 7.6.0* 
0031382: Data Exchange - BinXCAF should preserve length unit information
Currently, XCAF document (XBF) is loaded / written without length unit conversions, so that document implicitly preserves system units.
As different OCCT-based applications may choose different system units, opening the documents across such applications becomes problematic as there are no related information preserved in the file no standard tools for units conversion of XCAF document content.

As minimal requirement, it is necessary preserving length unit information within XBF/Xml file, which should be set and processed by unit-aware applications (by default, it might be reasonable keeping length unit UNDEFINED).
No tags attached.
related to 0031136reviewed bugmaster Open CASCADE Modeling Data - BinXCAF persistence loses normals from triangulation-only Faces 
Issue History
2020-02-18 17:30kgvNew Issue
2020-02-18 17:30kgvAssigned To => gka
2020-02-18 17:30kgvRelationship addedrelated to 0031136
2020-09-11 17:35gkaTarget Version7.5.0 => 7.6.0*
2020-10-01 10:32kgvSummary0031136: Modeling Data - BinXCAF should preserve length unit information => Modeling Data - BinXCAF should preserve length unit information
2020-10-01 11:35kgvSummaryModeling Data - BinXCAF should preserve length unit information => Data Exchange - BinXCAF should preserve length unit information
2020-10-29 09:45szyAssigned Togka => skl
2020-10-29 09:45szyStatusnew => assigned
2020-11-10 07:53gitNote Added: 0096632
2020-11-10 07:55gitNote Added: 0096633
2020-11-10 08:41gitNote Added: 0096634
2020-11-10 14:36gitNote Added: 0096638
2020-11-11 10:19gitNote Added: 0096652
2020-11-11 15:54sklNote Added: 0096659
2020-11-11 15:54sklAssigned Toskl => mpv
2020-11-11 15:54sklStatusassigned => resolved
2020-11-11 15:54sklSteps to Reproduce Updatedbug_revision_view_page.php?rev_id=23971#r23971
2020-11-11 17:01msvNote Added: 0096662
2020-11-11 17:05msvNote Added: 0096664
2020-11-11 17:11kgvNote Added: 0096666
2020-11-11 17:13kgvNote Edited: 0096666bug_revision_view_page.php?bugnote_id=96666#r23973
2020-11-11 17:13kgvNote Edited: 0096666bug_revision_view_page.php?bugnote_id=96666#r23974
2020-11-11 17:26msvNote Added: 0096668
2020-11-11 17:28mpvNote Added: 0096669
2020-11-11 17:28mpvAssigned Tompv => skl
2020-11-11 17:28mpvStatusresolved => assigned
2020-11-12 11:13gitNote Added: 0096690
2020-11-12 18:10sklNote Added: 0096726
2020-11-12 18:10sklAssigned Toskl => mpv
2020-11-12 18:10sklStatusassigned => resolved
2020-11-12 19:16kgvNote Added: 0096730
2020-11-13 17:50mpvAssigned Tompv => skl
2020-11-13 17:50mpvStatusresolved => assigned
2020-11-15 07:18gitNote Added: 0096778
2020-11-16 12:08gitNote Added: 0096788
2020-11-17 10:42gitNote Added: 0096818
2020-11-17 15:04sklNote Added: 0096828
2020-11-17 15:04sklAssigned Toskl => msv
2020-11-17 15:04sklStatusassigned => resolved
2020-11-17 15:10kgvNote Added: 0096829
2020-11-17 15:10kgvNote Edited: 0096829bug_revision_view_page.php?bugnote_id=96829#r24022
2020-11-17 15:55msvNote Added: 0096831
2020-11-17 15:55msvAssigned Tomsv => skl
2020-11-17 15:55msvStatusresolved => assigned
2020-11-17 16:07gitNote Added: 0096833
2020-11-18 07:02gitNote Added: 0096858
2020-11-18 11:34sklNote Added: 0096873
2020-11-18 11:34sklAssigned Toskl => msv
2020-11-18 11:34sklStatusassigned => resolved
2020-11-18 12:17msvNote Added: 0096878
2020-11-18 12:17msvAssigned Tomsv => skl
2020-11-18 12:17msvStatusresolved => assigned
2020-11-18 12:28gitNote Added: 0096880
2020-11-18 12:29sklAssigned Toskl => msv
2020-11-18 12:29sklStatusassigned => resolved
2020-11-18 12:32msvNote Added: 0096881
2020-11-18 12:32msvAssigned Tomsv => bugmaster
2020-11-18 12:32msvStatusresolved => reviewed
2020-11-19 10:56kgvNote Added: 0096891
2020-11-19 10:56kgvAssigned Tobugmaster => skl
2020-11-19 10:56kgvStatusreviewed => assigned
2020-11-19 10:59kgvNote Edited: 0096891bug_revision_view_page.php?bugnote_id=96891#r24037
2020-11-19 11:02kgvNote Edited: 0096891bug_revision_view_page.php?bugnote_id=96891#r24038
2020-11-19 15:11sklNote Added: 0096900
2020-11-19 15:11sklAssigned Toskl => kgv
2020-11-19 15:11sklStatusassigned => feedback
2020-11-20 10:00kgvRelationship addedrelated to 0031950
2020-11-24 15:27gitNote Added: 0097032
2020-12-10 10:33kgvNote Added: 0097453
2020-12-24 16:30gitNote Added: 0097891
2020-12-25 16:47gitNote Added: 0097906
2020-12-25 17:26gitNote Added: 0097907
2020-12-28 12:58sklAssigned Tokgv => skl
2020-12-28 12:58sklStatusfeedback => assigned
2020-12-28 13:03gitNote Added: 0097958
2020-12-28 16:52gitNote Added: 0097962
2020-12-28 18:59gitNote Added: 0097964
2020-12-29 09:25gitNote Added: 0097969
2020-12-30 11:00gitNote Added: 0097985
2020-12-31 10:18sklNote Added: 0097995
2020-12-31 10:18sklAssigned Toskl => msv
2020-12-31 10:18sklStatusassigned => resolved
2020-12-31 11:49msvNote Added: 0097997
2020-12-31 11:50msvAssigned Tomsv => skl
2020-12-31 11:50msvStatusresolved => assigned
2020-12-31 16:15gitNote Added: 0098000
2021-01-11 12:05sklNote Added: 0098050
2021-01-11 12:05sklAssigned Toskl => msv
2021-01-11 12:05sklStatusassigned => resolved
2021-01-11 14:11msvNote Added: 0098061
2021-01-11 14:12msvAssigned Tomsv => skl
2021-01-11 14:12msvStatusresolved => assigned
2021-01-11 14:23msvNote Added: 0098065
2021-01-11 14:25msvNote Deleted: 0098065
2021-01-11 14:26msvNote Added: 0098066
2021-01-11 14:28msvNote Added: 0098067
2021-01-11 15:19gitNote Added: 0098072
2021-01-11 15:22gitNote Added: 0098073
2021-01-12 10:04sklNote Added: 0098083
2021-01-12 10:04sklAssigned Toskl => msv
2021-01-12 10:04sklStatusassigned => resolved
2021-01-12 11:16kgvNote Added: 0098085
2021-01-12 11:24msvNote Added: 0098086
2021-01-12 11:24msvAssigned Tomsv => skl
2021-01-12 11:24msvStatusresolved => assigned
2021-01-13 12:37sklNote Added: 0098098
2021-01-13 12:37sklAssigned Toskl => msv
2021-01-13 12:37sklStatusassigned => resolved
2021-01-13 14:47msvNote Added: 0098103
2021-01-13 14:52msvNote Added: 0098104
2021-01-13 14:52msvAssigned Tomsv => bugmaster
2021-01-13 14:52msvStatusresolved => reviewed
2021-01-13 16:24kgvNote Added: 0098106
2021-01-13 16:24kgvAssigned Tobugmaster => skl
2021-01-13 16:24kgvStatusreviewed => assigned
2021-01-13 16:24kgvNote Edited: 0098106bug_revision_view_page.php?bugnote_id=98106#r24305
2021-01-13 16:25kgvNote Edited: 0098106bug_revision_view_page.php?bugnote_id=98106#r24306
2021-01-13 16:25kgvNote Edited: 0098106bug_revision_view_page.php?bugnote_id=98106#r24307
2021-01-13 16:25kgvNote Edited: 0098106bug_revision_view_page.php?bugnote_id=98106#r24308

2020-11-10 07:53   
Branch CR31382 has been created by skl.

SHA-1: 8638eb6923dd4c515bd8d899cf3c8f233b1734e7

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.
2020-11-10 07:55   
Branch CR31382 has been updated forcibly by skl.

SHA-1: 70bc6ab2cf4ecc77b2856b82f4324978a498398f
2020-11-10 08:41   
Branch CR31382 has been updated forcibly by skl.

SHA-1: d3ebb3870574430d07f8657fc4f45114ca82f8d0
2020-11-10 14:36   
Branch CR31382 has been updated forcibly by skl.

SHA-1: 3a4f47ea05b5faf8469cc349fc7d4fd8854effd8
2020-11-11 10:19   
Branch CR31382 has been updated forcibly by skl.

SHA-1: 4bdf1ebadccec4c4696508d5b294fbbf7ae561ab
2020-11-11 15:54   
Result of tests: [^]
2020-11-11 17:01   
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.
2020-11-11 17:05   
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.
2020-11-11 17:11   
(edited on: 2020-11-11 17:13)
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).

2020-11-11 17:26   
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.
2020-11-11 17:28   
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.
2020-11-12 11:13   
Branch CR31382 has been updated forcibly by skl.

SHA-1: e068e892f3134df99842e18d7881e87e2d22984a
2020-11-12 18:10   
Result of tests: [^]
2020-11-12 19:16   
+  enum LengthUnit {

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

+UnitsMethods::LengthUnit lengthUnitByStr(TCollection_AsciiString theStr)

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.
2020-11-15 07:18   
Branch CR31382 has been updated forcibly by skl.

SHA-1: ab1c68a13e150ae3ec6b1ba806d092de4001f640
2020-11-16 12:08   
Branch CR31382 has been updated forcibly by skl.

SHA-1: 763bab75800c94d58dae4db90953d1e9c98c6a5d
2020-11-17 10:42   
Branch CR31382 has been updated forcibly by skl.

SHA-1: cfd5bf689758ece85287db5ba9c9357138906e70
2020-11-17 15:04   
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: [^]

2020-11-17 15:10   
+  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

2020-11-17 15:55   
+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.
2020-11-17 16:07   
Branch CR31382 has been updated forcibly by skl.

SHA-1: 05da37a7aac569198019f4b4cf1a0508d426f8c1
2020-11-18 07:02   
Branch CR31382 has been updated forcibly by skl.

SHA-1: 7de4b54ab5b9f28877857fdde5f276e9e7845bc4
2020-11-18 11:34   
Result of tests: [^]
2020-11-18 12:17   
Please rebase on the latest master.
2020-11-18 12:28   
Branch CR31382 has been updated forcibly by skl.

SHA-1: cdaf43ee09e80fbc6d368d0c94b7fafb488142b5
2020-11-18 12:32   
For integration:
occt - CR31382
products - none
2020-11-19 10:56   
(edited on: 2020-11-19 11:02)
@@ -10,3 +10,5 @@ TKLCAF

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


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

+UnitsMethods_LengthUnit lengthUnitByStr(TCollection_AsciiString theStr)


+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 };

2020-11-19 15:11   
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?
2020-11-24 15:27   
Branch CR31382 has been updated forcibly by skl.

SHA-1: a1e2fe4eb0f4d439b71ea756b41793d9aeb1ca73
2020-12-10 10:33   
> 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.
2020-12-24 16:30   
Branch CR31382 has been updated forcibly by skl.

SHA-1: 0425c2c264d1563a48e08a386a50d9d9b6032f39
2020-12-25 16:47   
Branch CR31382 has been updated by skl.

SHA-1: 22a2d00cc3be85718bad5173f31d327326bb84bb

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.

2020-12-25 17:26   
Branch CR31382 has been updated forcibly by skl.

SHA-1: 3c5c03080bf878bed9c24bc86f6d6fbc00f254be
2020-12-28 13:03   
Branch CR31382 has been updated by skl.

SHA-1: e250344fe147c0b792f013f4231824e145d9c35a

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".

2020-12-28 16:52   
Branch CR31382 has been updated forcibly by skl.

SHA-1: dd51932a9680eb7c6f2ab685ffb639758f3966db
2020-12-28 18:59   
Branch CR31382 has been updated forcibly by skl.

SHA-1: 3e974687d560c8e8afe519b32f2de4b361ae7fbf
2020-12-29 09:25   
Branch CR31382 has been updated forcibly by skl.

SHA-1: 3c086c829ba4912cd6616d5beb31b6e695342469
2020-12-30 11:00   
Branch CR31382 has been updated forcibly by skl.

SHA-1: cccbf41f43b51078d31fb8c68cf8054468d312f5
2020-12-31 10:18   
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: [^]

2020-12-31 11:49   
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.
2020-12-31 16:15   
Branch CR31382 has been updated forcibly by skl.

SHA-1: f96ad714e2d84bf57b637cf82c60c9b2ccd992b3
2021-01-11 12:05   
Needed changes is done.
Result of tests: [^]
2021-01-11 14:11   
Again, the description of the method UnitsMethods::GetLengthUnitByFactorValue is not valid. Please correct it.
2021-01-11 14:26   
In XtControl_ActorRead::Transfer(), why did you commented out the following line? I think we should leave the call of this virtual method.
2021-01-11 14:28   
Prepare new ready for integration branches with one commit in both occt and products.
2021-01-11 15:19   
Branch CR31382 has been updated forcibly by skl.

SHA-1: cd6512ffde9a700723eafee98341526242620a48
2021-01-11 15:22   
Branch CR31382 has been updated forcibly by skl.

SHA-1: d4fd7db52d1ec856ceda766cd822c1e102f0ef53
2021-01-12 10:04   
Needed changes is done.
Result of tests: [^]
2021-01-12 11:16   
> 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

Please apply alphabetical order for new entities.

+class UnitsGeomMethods 

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


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).
2021-01-12 11:24   
  //! Returns the enumeration corresnoding to the given scale factor
Misprint "corresnoding"
2021-01-13 12:37   
Results of tests: [^]
2021-01-13 14:47   
I have corrected the spelling of commit message in CR31382_1.
2021-01-13 14:52   
For integration:
occt - CR31382_1
products - CR31382
2021-01-13 16:24   
(edited on: 2021-01-13 16:25)
+static Standard_Integer setLengthUnit(Draw_Interpretor& di, Standard_Integer argc, const char** 
+  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.