View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0028876Open CASCADEOCCT:Testspublic2017-06-28 09:482017-09-29 16:28
Reporterkgv Assigned Tobugmaster  
PrioritynormalSeverityminor 
Status closedResolutionfixed 
Product Version7.2.0 
Target Version7.2.0Fixed in Version7.2.0 
Summary0028876: Tests, Image_Diff - the image difference is unavailable for test case bugs vis bug28205_1
DescriptionThe image difference is unavailable for test case bugs vis bug28205_1.
The reason is that this test produces Grayscale images, which are not currently supported by Image_Diff tool.
Steps To Reproducetestdiff for bugs/vis/bug28205_1
TagsNo tags attached.
Test case numberNot required

Relationships

child of 0028205 closedapn Visualization - add functionality for dumping results of detection algorithms into image 
child of 0023272 closedkgv Image comparison algorithm 

Activities

git

2017-07-03 12:28

administrator   ~0067936

Branch CR28876 has been created by kgv.

SHA-1: 49f380640e8a14043b09edb031172154fe5c5669


Detailed log of new commits:

Author: kgv
Date: Mon Jul 3 12:27:08 2017 +0300

    0028876: Tests, Image_Diff - the image difference is unavailable for test case bugs vis bug28205_1
    
    Image_Diff has been improved to support Image_Format_Gray.
    Image_Diff::SaveDiffImage() now saves image difference
    in Image_Format_Gray format to reduce size of image file.

kgv

2017-07-03 15:56

developer   ~0067948

Last edited: 2017-07-03 15:58

 Patch is ready for review.

CR28876-master-KGV-Products-Windows-64-VC10-opt-tests-compare-windows - Build # 1 - Successful:
Check console output at http://jenkins-test-10.nnov.opencascade.com:8080/job/CR28876-master-KGV-Products-Windows-64-VC10-opt-tests-compare-windows/1/ to view the results.

CR28876-master-KGV-Products-Debian70-64-opt-tests-compare-linux - Build # 1 - Successful:
Check console output at http://jenkins-test-10.nnov.opencascade.com:8080/job/CR28876-master-KGV-Products-Debian70-64-opt-tests-compare-linux/1/ to view the results.

abv

2017-07-03 19:14

manager   ~0067954

Last edited: 2017-07-04 22:20

 I propose to revise the patch and some affected code to make it less confusing:

1. It is better to make method PixelColor() returning result by value instead of putting it in non-const reference argument; Quantity_ColorRGBA seems to be better choice than NCollection_Vec4f<float> since the latter does not have semantic of representing a color (). Related to this:

- Quantity_ColorRGBA deserves having constructor accepting 4 numbers
- Both Quantity_ColorRGBA and NCollection_Vec4f deserve having a setter function accepting 4 numbers
- Names of methods __testSize3() __testSize4() in Quantity_ColorRGBA violate C++ rules -- names with double underscore are reserved for compiler and standard library, see e.g. http://en.cppreference.com/w/cpp/language/identifiers

2. In Image_Diff::Compare(), add check for same size of the images before calling memcmp() to avoid possibility of accessing uninitialized memory if images are of different size

3. In Image_Diff.cxx, usage of Standard_Size for index of pixel seems to have no reason and leads to strange casts, better to be avoided (Standard_Integer seems to be quite sufficient).

4. In Image_Diff.cxx, method isBlack() of unnamed struct in "default" case of switch could call inline function isBlack(const NCollection_Vec4<float>&) defined just above instead of doing the same. The same for Gray and Alpha cases. Related to this:

- it is worth declaring inline functions defined in CXX file as static, to have compiler warning in case if they are not used

- it is better to give names even to locally used types (such as struct used in definition of NEIGHBOR_PIXELS -- this will make code more understandable (and also will allow compiler to generate more adequate warnings)

- for the same reason, I would convert methods of that structure to static functions
 
4. Putting textual messages to DefaultMessenger() (i.e. equivalent of cout) is not a good practice for a general-purpose tool... yet I see it possibly useful for our context of running it from DRAW... (this is hidden advertising of Alerts)

git

2017-07-04 13:50

administrator   ~0067970

Branch CR28876_1 has been created by kgv.

SHA-1: 30526377cac419d3453f446345c1178857cb7670


Detailed log of new commits:

Author: kgv
Date: Mon Jul 3 12:27:08 2017 +0300

    0028876: Tests, Image_Diff - the image difference is unavailable for test case bugs vis bug28205_1
    
    Quantity_ColorRGBA - added method SetValues().
    Image_PixMap::PixelColor() now returns Quantity_ColorRGBA instead of Quantity_Color.
    Image_PixMap::SetPixelColor() now takes Quantity_ColorRGBA instead of NCollection_Vec4<float>.
    
    Image_Diff has been improved to support Image_Format_Gray.
    Image_Diff::SaveDiffImage() now saves image difference
    in Image_Format_Gray format to reduce size of image file.
    
    Image_Diff now uses TColStd_HPackedMapOfInteger instead of
    TColStd_MapOfInteger with manual memory allocation.
    New struct DiffPixelVec2 has been defined for packing
    two 16-bit numbers into 32-bit integer.

kgv

2017-07-04 13:51

developer   ~0067971

Patch has been updated following remarks.

git

2017-07-04 14:21

administrator   ~0067973

Branch CR28876_1 has been updated forcibly by kgv.

SHA-1: ea0e194fc3d532f770a5c9dc9a441507731030e8

git

2017-07-05 03:06

administrator   ~0067981

Branch CR28876_1 has been updated by abv.

SHA-1: e6729670fe866a05f830250027bb7f9e1cf37f07


Detailed log of new commits:

Author: abv
Date: Wed Jul 5 02:39:59 2017 +0300

    # review: corrections to simplify the code

abv

2017-07-05 03:13

manager   ~0067982

I have made two corrections:

- in Image_Diff, avoid usage of NCollection_Vec2 and specific class DiffPixelVec2, using plain base types and simple functions instead

- in Quantity_ColorRGBA, avoid "explicit" in constructors accepting two arguments - here we seems to have no danger of possible unintended type conversion

Please look

git

2017-07-05 07:51

administrator   ~0067984

Branch CR28876_1 has been updated by kgv.

SHA-1: 4e3c0be106a4eb6538ff279255649e64ec967acd


Detailed log of new commits:

Author: kgv
Date: Wed Jul 5 07:51:31 2017 +0300

    # Image_Diff::SaveDiffImage() - fix parameters order passed to Image_PixMap::SetPixelColor()

git

2017-07-05 07:52

administrator   ~0067985

Branch CR28876_2 has been created by kgv.

SHA-1: 291fed9fdf750cccadeb077c9beb5522cada4faf


Detailed log of new commits:

Author: kgv
Date: Mon Jul 3 12:27:08 2017 +0300

    0028876: Tests, Image_Diff - the image difference is unavailable for test case bugs vis bug28205_1
    
    Quantity_ColorRGBA - added method SetValues().
    Image_PixMap::PixelColor() now returns Quantity_ColorRGBA instead of Quantity_Color.
    Image_PixMap::SetPixelColor() now takes Quantity_ColorRGBA instead of NCollection_Vec4<float>.
    
    Image_Diff has been improved to support Image_Format_Gray.
    Image_Diff::SaveDiffImage() now saves image difference
    in Image_Format_Gray format to reduce size of image file.
    
    Image_Diff now uses TColStd_HPackedMapOfInteger instead of
    TColStd_MapOfInteger with manual memory allocation.

kgv

2017-07-05 07:53

developer   ~0067986

Please test the patch.

git

2017-07-05 11:14

administrator   ~0067996

Branch CR28876_2 has been updated forcibly by kgv.

SHA-1: 2358a79c1bbddf9334b30bfdfddb8779bff3da6e

apv

2017-07-06 12:40

tester   ~0068032

Dear BugMaster,

Branch CR28876_2 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested.
SHA-1: 291fed9fdf750cccadeb077c9beb5522cada4faf

Number of compiler warnings:
occt component:
   Linux: 0 (0 on master)
   Windows: 0 (0 on master)
   MasOS: 0 (0 on master)
products component:
   Linux: 64
   Windows: 0
   MacOS: 1215

Regressions/Differences:
Not detected

Testing cases:
Not required

Testing on Linux:
Total MEMORY difference: 92768512 / 92186190 [+0.63%]
Total CPU difference: 18775.510000000166 / 18926.030000000348 [-0.80%]

Testing on Windows:
Total MEMORY difference: 58849153 / 58850236 [-0.00%]
Total CPU difference: 17507.898629398598 / 17597.770805498705 [-0.51%]

apv

2017-07-06 12:41

tester   ~0068033

Dear BugMaster,

Please integrate branch CR28876_2 into the occt git-repository master.

git

2017-07-10 08:58

administrator   ~0068126

Branch CR28876 has been deleted by kgv.

SHA-1: 49f380640e8a14043b09edb031172154fe5c5669

git

2017-07-10 08:58

administrator   ~0068127

Branch CR28876_1 has been deleted by kgv.

SHA-1: 4e3c0be106a4eb6538ff279255649e64ec967acd

git

2017-07-10 08:58

administrator   ~0068128

Branch CR28876_2 has been deleted by kgv.

SHA-1: 2358a79c1bbddf9334b30bfdfddb8779bff3da6e

Related Changesets

occt: master e958a649

2017-07-03 09:27:08

kgv


Committer: bugmaster Details Diff		 0028876: Tests, Image_Diff - the image difference is unavailable for test case bugs vis bug28205_1

Quantity_ColorRGBA - added method SetValues().
Image_PixMap::PixelColor() now returns Quantity_ColorRGBA instead of Quantity_Color.
Image_PixMap::SetPixelColor() now takes Quantity_ColorRGBA instead of NCollection_Vec4<float>.

Image_Diff has been improved to support Image_Format_Gray.
Image_Diff::SaveDiffImage() now saves image difference
in Image_Format_Gray format to reduce size of image file.

Image_Diff now uses TColStd_HPackedMapOfInteger instead of
TColStd_MapOfInteger with manual memory allocation.		 Affected Issues
0028876
mod - dox/dev_guides/upgrade/upgrade.md Diff File
mod - src/Graphic3d/Graphic3d_MarkerImage.cxx Diff File
mod - src/Image/Image_AlienPixMap.cxx Diff File
mod - src/Image/Image_Diff.cxx Diff File
mod - src/Image/Image_Diff.hxx Diff File
mod - src/Image/Image_PixMap.cxx Diff File
mod - src/Image/Image_PixMap.hxx Diff File
mod - src/NCollection/NCollection_Vec2.hxx Diff File
mod - src/NCollection/NCollection_Vec3.hxx Diff File
mod - src/NCollection/NCollection_Vec4.hxx Diff File
mod - src/Quantity/Quantity_ColorRGBA.hxx Diff File
mod - src/StdSelect/StdSelect_ViewerSelector3d.cxx Diff File
mod - src/ViewerTest/ViewerTest_ViewerCommands.cxx Diff File

Issue History

Date Modified Username Field Change
2017-06-28 09:48 kgv New Issue
2017-06-28 09:48 kgv Assigned To => kgv
2017-06-28 09:48 kgv Relationship added child of 0028205
2017-06-28 10:56 bugmaster Assigned To kgv => apn
2017-07-03 12:28 git Note Added: 0067936
2017-07-03 15:56 kgv Note Added: 0067948
2017-07-03 15:56 kgv Assigned To apn => abv
2017-07-03 15:56 kgv Status new => resolved
2017-07-03 15:56 kgv Relationship added child of 0023272
2017-07-03 15:57 kgv Note Edited: 0067948
2017-07-03 15:58 kgv Note Edited: 0067948
2017-07-03 19:14 abv Note Added: 0067954
2017-07-03 19:14 abv Assigned To abv => kgv
2017-07-03 19:14 abv Status resolved => feedback
2017-07-04 13:50 git Note Added: 0067970
2017-07-04 13:51 kgv Note Added: 0067971
2017-07-04 13:51 kgv Assigned To kgv => abv
2017-07-04 13:51 kgv Status feedback => resolved
2017-07-04 14:21 git Note Added: 0067973
2017-07-04 22:20 abv Note Edited: 0067954
2017-07-05 03:06 git Note Added: 0067981
2017-07-05 03:13 abv Note Added: 0067982
2017-07-05 03:13 abv Assigned To abv => kgv
2017-07-05 07:51 git Note Added: 0067984
2017-07-05 07:52 git Note Added: 0067985
2017-07-05 07:53 kgv Note Added: 0067986
2017-07-05 07:53 kgv Assigned To kgv => bugmaster
2017-07-05 07:53 kgv Status resolved => reviewed
2017-07-05 11:14 git Note Added: 0067996
2017-07-05 11:17 apv Assigned To bugmaster => apv
2017-07-05 14:34 apv Test case number => Not required
2017-07-06 12:40 apv Note Added: 0068032
2017-07-06 12:40 apv Assigned To apv => bugmaster
2017-07-06 12:40 apv Status reviewed => tested
2017-07-06 12:41 apv Note Added: 0068033
2017-07-07 17:25 bugmaster Changeset attached => occt master e958a649
2017-07-07 17:25 bugmaster Status tested => verified
2017-07-07 17:25 bugmaster Resolution open => fixed
2017-07-10 08:58 git Note Added: 0068126
2017-07-10 08:58 git Note Added: 0068127
2017-07-10 08:58 git Note Added: 0068128
2017-09-29 16:18 aiv Fixed in Version => 7.2.0
2017-09-29 16:28 aiv Status verified => closed