Archived issue #0023493
Incorrect QAGetPixelColor usage
Description
QAGetPixelColor command was designed to obtain pixel color for specified position in 3D viewer and:
- Print components in format "RED : 0.93333333730697632 GREEN : 0 BLUE : 0".
- Compare color with specified by user (as 3 additional arguments). If colors aren't equal than command returns 1 (initiate exception) and prints extra message "Faulty : colors are not equal.".
There are several issues within this command and its usage in tests:
- Output format of this command was changed since #0019862 (originally "Faulty" keyword was printed at the beginning but now - after color components output) however some test cases perform checks like that (chl 902 R3):
> catch {QAGetPixelColor 89 363 0.933333 0 0} result
> set err [lindex $result 0]
> if {$err != "Faulty"} {
- Some tests (chl 919 C9) ignores the fact that QAGetPixelColor returns 1 when comparison is failed. Thus such code looks meaningless because it will not be executed in fault case (no catch):
> set r [QAGetPixelColor ${x1} ${y1} ${BLACK_R} ${BLACK_G} ${BLACK_B}]
> if {[lindex $r 0] == "Faulty"} {
> puts " OCC173 OK: vsettransparency of shape was made"
> } else {
> puts "OCC173 Faulty : vsettransparency of shape was NOT made"
> }
- Linux-specific comparison code:
> if {$station == "lin"} {
> catch {QAGetPixelColor 88 316} result
> set red_lin [lindex $result 2]
> set green_lin [lindex $result 5]
> set blue_lin [lindex $result 8]
> if {${red_lin} == 0
> || [expr abs(${green_lin} - 0.8) < 0.1]
> || [expr abs(${blue_lin} - 0.8) < 0.1]} {
> set err "OK"
> }
> }
This is unknown why these checks are Linux-specific but anyway them looks very strange because only single RGB component of color required to be equal (OR instead of AND).
- It seems that output of QAGetPixelColor wasn't designed to be parsed in such way. This is better to use vreadpixel command instead:
> catch {vreadpixel 200 200 rgb} aColor
> set aRed [lindex $aColor 0]
> set aGreen [lindex $aColor 1]
> set aBlue [lindex $aColor 2]
My suggestion is to analyze all cases where QAGetPixelColor currently used for common mistakes and probably replace comparison functionality with new command like vcheckpixel with more reliable usage syntax.
- Print components in format "RED : 0.93333333730697632 GREEN : 0 BLUE : 0".
- Compare color with specified by user (as 3 additional arguments). If colors aren't equal than command returns 1 (initiate exception) and prints extra message "Faulty : colors are not equal.".
There are several issues within this command and its usage in tests:
- Output format of this command was changed since #0019862 (originally "Faulty" keyword was printed at the beginning but now - after color components output) however some test cases perform checks like that (chl 902 R3):
> catch {QAGetPixelColor 89 363 0.933333 0 0} result
> set err [lindex $result 0]
> if {$err != "Faulty"} {
- Some tests (chl 919 C9) ignores the fact that QAGetPixelColor returns 1 when comparison is failed. Thus such code looks meaningless because it will not be executed in fault case (no catch):
> set r [QAGetPixelColor ${x1} ${y1} ${BLACK_R} ${BLACK_G} ${BLACK_B}]
> if {[lindex $r 0] == "Faulty"} {
> puts " OCC173 OK: vsettransparency of shape was made"
> } else {
> puts "OCC173 Faulty : vsettransparency of shape was NOT made"
> }
- Linux-specific comparison code:
> if {$station == "lin"} {
> catch {QAGetPixelColor 88 316} result
> set red_lin [lindex $result 2]
> set green_lin [lindex $result 5]
> set blue_lin [lindex $result 8]
> if {${red_lin} == 0
> || [expr abs(${green_lin} - 0.8) < 0.1]
> || [expr abs(${blue_lin} - 0.8) < 0.1]} {
> set err "OK"
> }
> }
This is unknown why these checks are Linux-specific but anyway them looks very strange because only single RGB component of color required to be equal (OR instead of AND).
- It seems that output of QAGetPixelColor wasn't designed to be parsed in such way. This is better to use vreadpixel command instead:
> catch {vreadpixel 200 200 rgb} aColor
> set aRed [lindex $aColor 0]
> set aGreen [lindex $aColor 1]
> set aBlue [lindex $aColor 2]
My suggestion is to analyze all cases where QAGetPixelColor currently used for common mistakes and probably replace comparison functionality with new command like vcheckpixel with more reliable usage syntax.
Steps to reproduce
Not required
Public activity
21 archived notes
Participants are labeled by their role within this record.
Branch [archived branch] has been created by Participant.
[revision removed]
Detailed log of new commits:
Author: ski
Date: Fri Nov 28 16:43:27 2014 +0300
0023493: Incorrect QAGetPixelColor usage
Usage of QAGetPixelColor were checked and corrected.
[revision removed]
Detailed log of new commits:
Author: ski
Date: Fri Nov 28 16:43:27 2014 +0300
0023493: Incorrect QAGetPixelColor usage
Usage of QAGetPixelColor were checked and corrected.
Branch [archived branch] has been updated forcibly by Participant.
[revision removed]
[revision removed]
Usage of command QAGetPixelColor in tests were checked.
Some tests were simplified by using command vreadpixel.
Changed test cases were tested on win32 vc9.
Please, review.
Some tests were simplified by using command vreadpixel.
Changed test cases were tested on win32 vc9.
Please, review.
+ set Color1 [vreadpixel ${x1} ${i}]
+ regexp {^([-0-9.+eE]+) ([-0-9.+eE]+) ([-0-9.+eE]+)} $Color1 full R1 G1 B1
is it really necessary / preferable to use regexp everywhere instead of simpler comparisons like suggested in the description?
> catch {vreadpixel 200 200 rgb} aColor
> set aRed [lindex $aColor 0]
> set aGreen [lindex $aColor 1]
> set aBlue [lindex $aColor 2]
+if { [regexp {^0 0 0} [vreadpixel $x1 $y1 rgb]] } {
+ puts "Faulty ${BugNumber}"
some cases might become simpler if replaced by color name comparison "[vreadpixel $x1 $y1 rgb name]" == "BLACK".
Branch [archived branch] has been updated by Participant.
[revision removed]
Detailed log of new commits:
Author: ski
Date: Tue Dec 2 18:21:27 2014 +0300
Using simple comparison instead of regexp.
[revision removed]
Detailed log of new commits:
Author: ski
Date: Tue Dec 2 18:21:27 2014 +0300
Using simple comparison instead of regexp.
Remarks were applied.
It seems that in test case bugs / vis / bug232 it is easier to use regexp instead of any other compare method.
All changes were tested on win32 vc9.
Please, review.
It seems that in test case bugs / vis / bug232 it is easier to use regexp instead of any other compare method.
All changes were tested on win32 vc9.
Please, review.
+set BlackColor "0 0 0"
+
+if { "[vreadpixel $x1 $y1 rgb]" == "$BlackColor" } {
this is not what I have meant - there is option "name" for vreadpixel command which allows to print color name instead of RGB values like this:
set aColorName [vreadpixel $x1 $y1 rgb name]
if { "$aColorName" == "BLACK" }
Branch [archived branch] has been updated by Participant.
[revision removed]
Detailed log of new commits:
Author: ski
Date: Tue Dec 2 19:51:26 2014 +0300
Improved usage of command vreadpixel for standard colors.
[revision removed]
Detailed log of new commits:
Author: ski
Date: Tue Dec 2 19:51:26 2014 +0300
Improved usage of command vreadpixel for standard colors.
Usage of command vreadpixel for standard colors was improved.
QAGetPixelColor is now used only within auxiliary procedures "checkpoint" and "checkcolor". It make sense to replace remaining occurrences by vreadpixel and drop QAGetPixelColor from TKQADraw at all.
The procedure "checkcolor" is defined 4 times in test base (2 times in occt and 2 times in products). I suggest to share implementation (move it to DrawResources/TestCommands.tcl?). Note that QAAISGetPixelColor2d do not exists anymore, thus the corresponding branches in "checkcolor" should be dropped.
By the way, color-picking issues due to different hardware might be mitigated by specifying extra width to presentation. E.g. for picking line in test case "vaspects prs -setwidth 4" (though there is no alternative for markers yet until #0025288 and #0025350). It would be good to analyze test cases using "checkcolor" for picking line color and try this trick.
I suppose this particular test case has not been checked (without "rgb" command vreadpixel would return also alpha value "BLACK 0").
The procedure "checkcolor" is defined 4 times in test base (2 times in occt and 2 times in products). I suggest to share implementation (move it to DrawResources/TestCommands.tcl?). Note that QAAISGetPixelColor2d do not exists anymore, thus the corresponding branches in "checkcolor" should be dropped.
By the way, color-picking issues due to different hardware might be mitigated by specifying extra width to presentation. E.g. for picking line in test case "vaspects prs -setwidth 4" (though there is no alternative for markers yet until #0025288 and #0025350). It would be good to analyze test cases using "checkcolor" for picking line color and try this trick.
- if { "[vreadpixel ${x1} ${i}]" == "$BlackColor" } {
+ if { "[vreadpixel ${x1} ${i} name]" == "BLACK" } {
I suppose this particular test case has not been checked (without "rgb" command vreadpixel would return also alpha value "BLACK 0").
Branch [archived branch] has been updated by Participant.
[revision removed]
Detailed log of new commits:
Author: ski
Date: Mon Dec 8 18:26:53 2014 +0300
Some test cases using "checkcolor" for picking line color were simplified.
Procedures checkcolor and checkpoint were changed to handle situation when pixel is out of view.
Author: ski
Date: Wed Dec 3 17:55:28 2014 +0300
Command QAGetPixelColor was dropped from TKQADraw.
Procedures "checkcolor" and auxiliary "checkpoint" were moved to DrawResources/TestCommands.tcl
[revision removed]
Detailed log of new commits:
Author: ski
Date: Mon Dec 8 18:26:53 2014 +0300
Some test cases using "checkcolor" for picking line color were simplified.
Procedures checkcolor and checkpoint were changed to handle situation when pixel is out of view.
Author: ski
Date: Wed Dec 3 17:55:28 2014 +0300
Command QAGetPixelColor was dropped from TKQADraw.
Procedures "checkcolor" and auxiliary "checkpoint" were moved to DrawResources/TestCommands.tcl
Branch [archived branch] has been updated forcibly by Participant.
[revision removed]
[revision removed]
Branch CR23493 was rebased on current master.
Product branch CR23493 was created.
All remarks were applied.
I had used "vaspects -unsetwidth" command at the end of tests to get more clearer test cases vdump.
Changed test cases were tested on win32 vc9 and Debian60-64.
Please, review.
Product branch CR23493 was created.
All remarks were applied.
I had used "vaspects -unsetwidth" command at the end of tests to get more clearer test cases vdump.
Changed test cases were tested on win32 vc9 and Debian60-64.
Please, review.
+vaspects -unsetwidth
Please consider taking final screenshots as is, with increased width for consistency.
+#checkcolor $x_coord $y_coord 0.93 0 0
+if {"[vreadpixel $x_coord $y_coord rgb name]" != "RED2"} {
Commented line looks redundant.
Branch [archived branch] has been updated by Participant.
[revision removed]
Detailed log of new commits:
Author: ski
Date: Tue Dec 9 16:41:28 2014 +0300
Removed unnecessary use of command "vaspects -setwidth" in tests.
[revision removed]
Detailed log of new commits:
Author: ski
Date: Tue Dec 9 16:41:28 2014 +0300
Removed unnecessary use of command "vaspects -setwidth" in tests.
Minor remarks were applied.
Following test cases will have differences in images (line width is increased):
bugs / vis / bug74
bugs / vis / bug7186
bugs / vis / bug60
bugs / vis / bug349_1
bugs / vis / bug349
bugs / vis / bug24867
bugs / vis / bug24717
bugs / vis / bug24714
bugs / vis / bug24564
bugs / vis / bug24391
bugs / vis / bug23709_4
bugs / vis / bug23709_3
bugs / vis / bug23709_2
bugs / vis / bug23709_1
bugs / vis / bug23525
bugs / vis / bug23407_2
bugs / vis / bug11615
bugs / vis / buc60688
Note that product branch CR23493 exists.
Following test cases will have differences in images (line width is increased):
bugs / vis / bug74
bugs / vis / bug7186
bugs / vis / bug60
bugs / vis / bug349_1
bugs / vis / bug349
bugs / vis / bug24867
bugs / vis / bug24717
bugs / vis / bug24714
bugs / vis / bug24564
bugs / vis / bug24391
bugs / vis / bug23709_4
bugs / vis / bug23709_3
bugs / vis / bug23709_2
bugs / vis / bug23709_1
bugs / vis / bug23525
bugs / vis / bug23407_2
bugs / vis / bug11615
bugs / vis / buc60688
Note that product branch CR23493 exists.
Dear Commenter 1,
Branch CR23493 from occt git-repository and CR23493from products git-repository were compiled on Linux, MacOS and Windows platforms and tested on Release mode.
[revision removed]
[revision removed]
Number of compiler warnings:
occt component :
Linux: 18 (18 on master)
Windows: 0 (0 on master)
products component :
Linux: 11 (11 on master)
Windows: 1 (1 on master)
Regressions/Differences:
No regressions/differences
Testing cases:
Not needed
Testing on Linux:
occt component :
Total MEMORY difference: 363615872 / 363562240
Total CPU difference: 47870.45000000011 / 47537.4299999999
products component :
Total MEMORY difference: 111705008 / 111872212
Total CPU difference: 16025.420000000002 / 15946.739999999983
Testing on Windows:
occt component :
Total MEMORY difference: 276211196 / 277181316
Total CPU difference: 34273.921875 / 41681.890625
products component :
Total MEMORY difference: 72138620 / 71216368
Total CPU difference: 11328.9375 / 13037.484375
There are following differences in images found by testdiff.
http://occt-tests.nnov.opencascade.com/CR23493-CR23493-occt/Debian60-64/diff-Debian60-64.html
http://occt-tests.nnov.opencascade.com/CR23493-CR23493-occt/Windows-32-VC10/diff-Windows-32-VC10.html
IMAGE bugs vis bug24717: bug24717.png differs
IMAGE bugs vis bug60: bug60_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug24391: bug24391_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug24714: bug24714_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug71: bug7186_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug23525: bug23525_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug7186: bug7186_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug23709_3: bug23709_3_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug23709_2: bug23709_2_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug349: bug349_1_Driver1_Viewer1_View1_new.png differs
IMAGE bugs vis bug349: bug349_Driver1_Viewer1_View1_new.png differs
IMAGE bugs vis bug23709_4: bug23709_4_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug349_1: bug349_1_Driver1_Viewer1_View1_new.png differs
IMAGE bugs vis bug23709_1: bug23709_1_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug24867: bug24867_selected.png differs
IMAGE bugs vis bug24867: bug24867_highlighted.png differs
IMAGE bugs vis buc60688: buc60688_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug11615: bug11615_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug74: bug74_Driver1_Viewer1_View1.png differs
Branch CR23493 from occt git-repository and CR23493from products git-repository were compiled on Linux, MacOS and Windows platforms and tested on Release mode.
[revision removed]
[revision removed]
Number of compiler warnings:
occt component :
Linux: 18 (18 on master)
Windows: 0 (0 on master)
products component :
Linux: 11 (11 on master)
Windows: 1 (1 on master)
Regressions/Differences:
No regressions/differences
Testing cases:
Not needed
Testing on Linux:
occt component :
Total MEMORY difference: 363615872 / 363562240
Total CPU difference: 47870.45000000011 / 47537.4299999999
products component :
Total MEMORY difference: 111705008 / 111872212
Total CPU difference: 16025.420000000002 / 15946.739999999983
Testing on Windows:
occt component :
Total MEMORY difference: 276211196 / 277181316
Total CPU difference: 34273.921875 / 41681.890625
products component :
Total MEMORY difference: 72138620 / 71216368
Total CPU difference: 11328.9375 / 13037.484375
There are following differences in images found by testdiff.
http://occt-tests.nnov.opencascade.com/CR23493-CR23493-occt/Debian60-64/diff-Debian60-64.html
http://occt-tests.nnov.opencascade.com/CR23493-CR23493-occt/Windows-32-VC10/diff-Windows-32-VC10.html
IMAGE bugs vis bug24717: bug24717.png differs
IMAGE bugs vis bug60: bug60_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug24391: bug24391_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug24714: bug24714_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug71: bug7186_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug23525: bug23525_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug7186: bug7186_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug23709_3: bug23709_3_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug23709_2: bug23709_2_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug349: bug349_1_Driver1_Viewer1_View1_new.png differs
IMAGE bugs vis bug349: bug349_Driver1_Viewer1_View1_new.png differs
IMAGE bugs vis bug23709_4: bug23709_4_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug349_1: bug349_1_Driver1_Viewer1_View1_new.png differs
IMAGE bugs vis bug23709_1: bug23709_1_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug24867: bug24867_selected.png differs
IMAGE bugs vis bug24867: bug24867_highlighted.png differs
IMAGE bugs vis buc60688: buc60688_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug11615: bug11615_Driver1_Viewer1_View1.png differs
IMAGE bugs vis bug74: bug74_Driver1_Viewer1_View1.png differs
Please revert -setwidth change in test bugs/vis/bug23525, other images look fine.
Branch [archived branch] has been updated by Participant.
[revision removed]
Detailed log of new commits:
Author: ski
Date: Thu Dec 11 11:06:13 2014 +0300
Revert -setwidth change in test bugs/vis/bug23525
[revision removed]
Detailed log of new commits:
Author: ski
Date: Thu Dec 11 11:06:13 2014 +0300
Revert -setwidth change in test bugs/vis/bug23525
Changes in test bugs/vis/bug23525 were reverted.
Test and image are OK on win32 vc9 and Debian60-64.
Test and image are OK on win32 vc9 and Debian60-64.
Branch [archived branch] has been deleted by Participant.
[revision removed]
[revision removed]
Related records