MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0031382Open CASCADE[OCCT] OCCT:Data Exchangepublic2020-02-18 17:302020-11-24 15:27
Reporterkgv 
Assigned Tokgv 
PrioritynormalSeverityfeature 
StatusfeedbackResolutionopen 
PlatformOSOS Version
Product Version 
Target Version[OCCT] 7.6.0*Fixed in Version 
Summary0031382: Data Exchange - BinXCAF should preserve length unit information
DescriptionCurrently, 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).
Steps To Reproducebug31382
TagsNo tags attached.
Test case number
Attached Files

- Relationships
related to 0031136feedbackasuraven Open CASCADE Modeling Data - BinXCAF persistence loses normals from triangulation-only Faces 

-  Notes
(0096632)
git (administrator)
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.
(0096633)
git (administrator)
2020-11-10 07:55

Branch CR31382 has been updated forcibly by skl.

SHA-1: 70bc6ab2cf4ecc77b2856b82f4324978a498398f
(0096634)
git (administrator)
2020-11-10 08:41

Branch CR31382 has been updated forcibly by skl.

SHA-1: d3ebb3870574430d07f8657fc4f45114ca82f8d0
(0096638)
git (administrator)
2020-11-10 14:36

Branch CR31382 has been updated forcibly by skl.

SHA-1: 3a4f47ea05b5faf8469cc349fc7d4fd8854effd8
(0096652)
git (administrator)
2020-11-11 10:19

Branch CR31382 has been updated forcibly by skl.

SHA-1: 4bdf1ebadccec4c4696508d5b294fbbf7ae561ab
(0096659)
skl (developer)
2020-11-11 15:54

Result of tests:
http://vm-jenkins-test-12.nnov.opencascade.com:8080/view/CR31382-master/view/COMPARE/ [^]
(0096662)
msv (developer)
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.
(0096664)
msv (developer)
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.
(0096666)
kgv (developer)
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).

(0096668)
msv (developer)
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.
(0096669)
mpv (developer)
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.
(0096690)
git (administrator)
2020-11-12 11:13

Branch CR31382 has been updated forcibly by skl.

SHA-1: e068e892f3134df99842e18d7881e87e2d22984a
(0096726)
skl (developer)
2020-11-12 18:10

Result of tests:
http://vm-jenkins-test-12.nnov.opencascade.com:8080/view/CR31382-master/view/COMPARE/ [^]
(0096730)
kgv (developer)
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)

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.
(0096778)
git (administrator)
2020-11-15 07:18

Branch CR31382 has been updated forcibly by skl.

SHA-1: ab1c68a13e150ae3ec6b1ba806d092de4001f640
(0096788)
git (administrator)
2020-11-16 12:08

Branch CR31382 has been updated forcibly by skl.

SHA-1: 763bab75800c94d58dae4db90953d1e9c98c6a5d
(0096818)
git (administrator)
2020-11-17 10:42

Branch CR31382 has been updated forcibly by skl.

SHA-1: cfd5bf689758ece85287db5ba9c9357138906e70
(0096828)
skl (developer)
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:
http://vm-jenkins-test-12.nnov.opencascade.com:8080/view/CR31382-master/view/COMPARE/ [^]

(0096829)
kgv (developer)
2020-11-17 15:10
edited on: 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

(0096831)
msv (developer)
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.
(0096833)
git (administrator)
2020-11-17 16:07

Branch CR31382 has been updated forcibly by skl.

SHA-1: 05da37a7aac569198019f4b4cf1a0508d426f8c1
(0096858)
git (administrator)
2020-11-18 07:02

Branch CR31382 has been updated forcibly by skl.

SHA-1: 7de4b54ab5b9f28877857fdde5f276e9e7845bc4
(0096873)
skl (developer)
2020-11-18 11:34

Result of tests:
http://vm-jenkins-test-12.nnov.opencascade.com:8080/view/CR31382-master/view/COMPARE/ [^]
(0096878)
msv (developer)
2020-11-18 12:17

Please rebase on the latest master.
(0096880)
git (administrator)
2020-11-18 12:28

Branch CR31382 has been updated forcibly by skl.

SHA-1: cdaf43ee09e80fbc6d368d0c94b7fafb488142b5
(0096881)
msv (developer)
2020-11-18 12:32

For integration:
occt - CR31382
products - none
(0096891)
kgv (developer)
2020-11-19 10:56
edited on: 2020-11-19 11:02

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

(0096900)
skl (developer)
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?
(0097032)
git (administrator)
2020-11-24 15:27

Branch CR31382 has been updated forcibly by skl.

SHA-1: a1e2fe4eb0f4d439b71ea756b41793d9aeb1ca73

- Issue History
Date Modified Username Field Change
2020-02-18 17:30 kgv New Issue
2020-02-18 17:30 kgv Assigned To => gka
2020-02-18 17:30 kgv Relationship added related to 0031136
2020-09-11 17:35 gka Target Version 7.5.0 => 7.6.0*
2020-10-01 10:32 kgv Summary 0031136: Modeling Data - BinXCAF should preserve length unit information => Modeling Data - BinXCAF should preserve length unit information
2020-10-01 11:35 kgv Summary Modeling Data - BinXCAF should preserve length unit information => Data Exchange - BinXCAF should preserve length unit information
2020-10-29 09:45 szy Assigned To gka => skl
2020-10-29 09:45 szy Status new => assigned
2020-11-10 07:53 git Note Added: 0096632
2020-11-10 07:55 git Note Added: 0096633
2020-11-10 08:41 git Note Added: 0096634
2020-11-10 14:36 git Note Added: 0096638
2020-11-11 10:19 git Note Added: 0096652
2020-11-11 15:54 skl Note Added: 0096659
2020-11-11 15:54 skl Assigned To skl => mpv
2020-11-11 15:54 skl Status assigned => resolved
2020-11-11 15:54 skl Steps to Reproduce Updated View Revisions
2020-11-11 17:01 msv Note Added: 0096662
2020-11-11 17:05 msv Note Added: 0096664
2020-11-11 17:11 kgv Note Added: 0096666
2020-11-11 17:13 kgv Note Edited: 0096666 View Revisions
2020-11-11 17:13 kgv Note Edited: 0096666 View Revisions
2020-11-11 17:26 msv Note Added: 0096668
2020-11-11 17:28 mpv Note Added: 0096669
2020-11-11 17:28 mpv Assigned To mpv => skl
2020-11-11 17:28 mpv Status resolved => assigned
2020-11-12 11:13 git Note Added: 0096690
2020-11-12 18:10 skl Note Added: 0096726
2020-11-12 18:10 skl Assigned To skl => mpv
2020-11-12 18:10 skl Status assigned => resolved
2020-11-12 19:16 kgv Note Added: 0096730
2020-11-13 17:50 mpv Assigned To mpv => skl
2020-11-13 17:50 mpv Status resolved => assigned
2020-11-15 07:18 git Note Added: 0096778
2020-11-16 12:08 git Note Added: 0096788
2020-11-17 10:42 git Note Added: 0096818
2020-11-17 15:04 skl Note Added: 0096828
2020-11-17 15:04 skl Assigned To skl => msv
2020-11-17 15:04 skl Status assigned => resolved
2020-11-17 15:10 kgv Note Added: 0096829
2020-11-17 15:10 kgv Note Edited: 0096829 View Revisions
2020-11-17 15:55 msv Note Added: 0096831
2020-11-17 15:55 msv Assigned To msv => skl
2020-11-17 15:55 msv Status resolved => assigned
2020-11-17 16:07 git Note Added: 0096833
2020-11-18 07:02 git Note Added: 0096858
2020-11-18 11:34 skl Note Added: 0096873
2020-11-18 11:34 skl Assigned To skl => msv
2020-11-18 11:34 skl Status assigned => resolved
2020-11-18 12:17 msv Note Added: 0096878
2020-11-18 12:17 msv Assigned To msv => skl
2020-11-18 12:17 msv Status resolved => assigned
2020-11-18 12:28 git Note Added: 0096880
2020-11-18 12:29 skl Assigned To skl => msv
2020-11-18 12:29 skl Status assigned => resolved
2020-11-18 12:32 msv Note Added: 0096881
2020-11-18 12:32 msv Assigned To msv => bugmaster
2020-11-18 12:32 msv Status resolved => reviewed
2020-11-19 10:56 kgv Note Added: 0096891
2020-11-19 10:56 kgv Assigned To bugmaster => skl
2020-11-19 10:56 kgv Status reviewed => assigned
2020-11-19 10:59 kgv Note Edited: 0096891 View Revisions
2020-11-19 11:02 kgv Note Edited: 0096891 View Revisions
2020-11-19 15:11 skl Note Added: 0096900
2020-11-19 15:11 skl Assigned To skl => kgv
2020-11-19 15:11 skl Status assigned => feedback
2020-11-20 10:00 kgv Relationship added related to 0031950
2020-11-24 15:27 git Note Added: 0097032


Copyright © 2000 - 2020 MantisBT Team
Powered by Mantis Bugtracker