MantisBT - Community
View Issue Details
0023156Community[OCCT] OCCT:Visualizationpublic2012-05-15 10:362012-11-16 13:16
vro 
san 
normalminor 
closedfixed 
WindowsVC++ 200532 bit
[OCCT] 6.5.3 
[OCCT] 6.5.4[OCCT] 6.5.4 
chl 934 E0
0023156: A class Image_PixMap doesn't give direct access to transparency information
A class Image_PixMap has a method PixelColor(). It returns color of a pixel. But if the user needs the information on transparency of the pixel, he has to explore a raw data set. It seems complicated and not convenient.
A new method PixelColorWithAlpha() might solve this problem. It might be an additional, non-virtual method returning a transparency value in addition to color.
No tags attached.
? E0 (7,708) 2012-06-14 17:36
https://tracker.dev.opencascade.org/
Issue History
2012-05-15 10:36vroNew Issue
2012-05-15 10:36vroAssigned To => vro
2012-05-15 10:50vroAssigned Tovro => san
2012-05-15 10:51vroNote Added: 0020534
2012-05-15 10:51vroStatusnew => resolved
2012-05-22 19:08sanNote Added: 0020566
2012-05-22 19:08sanAssigned Tosan => vro
2012-05-22 19:08sanStatusresolved => assigned
2012-05-23 10:30vroNote Added: 0020567
2012-05-23 10:30vroAssigned Tovro => san
2012-05-23 10:30vroStatusassigned => resolved
2012-05-23 13:34sanNote Added: 0020569
2012-05-23 13:34sanAssigned Tosan => vro
2012-05-23 13:34sanStatusresolved => assigned
2012-05-29 14:50vroNote Added: 0020599
2012-05-29 14:50vroAssigned Tovro => san
2012-05-29 14:50vroStatusassigned => resolved
2012-05-29 17:57sanNote Added: 0020600
2012-05-29 17:57sanAssigned Tosan => bugmaster
2012-05-29 17:57sanStatusresolved => reviewed
2012-05-29 18:26mkvAssigned Tobugmaster => aan
2012-05-29 18:45kgvNote Added: 0020601
2012-05-29 19:03kgvNote Added: 0020602
2012-05-29 19:03kgvNote Edited: 0020602bug_revision_view_page.php?bugnote_id=20602#r3966
2012-05-29 19:04kgvNote Edited: 0020602bug_revision_view_page.php?bugnote_id=20602#r3967
2012-05-29 19:08sanNote Added: 0020603
2012-05-29 19:08sanAssigned Toaan => vro
2012-05-29 19:08sanStatusreviewed => assigned
2012-06-07 15:14vroNote Added: 0020657
2012-06-07 15:14vroAssigned Tovro => kgv
2012-06-07 15:14vroStatusassigned => resolved
2012-06-08 11:51kgvAssigned Tokgv => san
2012-06-08 11:53kgvNote Added: 0020668
2012-06-08 12:23vroNote Added: 0020671
2012-06-08 13:13sanNote Added: 0020674
2012-06-08 13:13sanAssigned Tosan => bugmaster
2012-06-08 13:13sanStatusresolved => reviewed
2012-06-08 14:24mkvAssigned Tobugmaster => mkv
2012-06-08 15:16sanNote Added: 0020675
2012-06-08 15:45mkvNote Added: 0020678
2012-06-14 17:35mkvNote Added: 0020695
2012-06-14 17:35mkvNote Added: 0020696
2012-06-14 17:36mkvFile Added: E0
2012-06-14 17:37mkvTest case number => chl 934 E0
2012-06-14 17:37mkvAssigned Tomkv => kgv
2012-06-14 17:37mkvStatusreviewed => assigned
2012-06-15 17:05kgvNote Added: 0020710
2012-06-19 22:07kgvAssigned Tokgv => mkv
2012-06-21 15:29mkvNote Added: 0020747
2012-06-21 15:29mkvAssigned Tomkv => bugmaster
2012-06-21 15:30mkvStatusassigned => resolved
2012-06-21 15:30mkvStatusresolved => reviewed
2012-06-21 15:30mkvStatusreviewed => tested
2012-06-25 20:05sanChangeset attached => occt master 85e096c3
2012-06-25 20:05sanAssigned Tobugmaster => san
2012-06-25 20:05sanStatustested => verified
2012-06-25 20:05sanResolutionopen => fixed
2012-11-16 13:15bugmasterFixed in Version => 6.5.4
2012-11-16 13:16bugmasterStatusverified => closed

Notes
(0020534)
vro   
2012-05-15 10:51   
A Git branch CR23156 contains a new method Image_PixMap::PixelColorWithAlpha().
Could you review the patch please?
(0020566)
san   
2012-05-22 19:08   
Branch CR23156 reviewed with the following remarks.

Image_PixMap.cxx, line 484:
here method Image_PixMap::PixelColorWithAlpha() returns without initializing theAlpha output argument, while in all other cases some value is assigned to theAlpha. This potentially might result in reading uninitialized float value at application side, because PixelColorWithAlpha() does not provide any means to detect such a situation. It is suggested to assign some default value to theAlpha in any case or make PixelColorWithAlpha() return Boolean status to signal that the pixel color and the alpha value have not been retrieved.
(0020567)
vro   
2012-05-23 10:30   
Fixed. Thank you for so careful review!
(0020569)
san   
2012-05-23 13:34   
What about the DRAW command for testing new Image_PixMap method?
Without it the test case cannot be created for this issue.
(0020599)
vro   
2012-05-29 14:50   
A draw-command VGetPixelColorWithAlpha is added.
(0020600)
san   
2012-05-29 17:57   
CR23156 reviewed without remarks.

Please, contact the patch author for details how to obtain non-default alpha values with help of the new command, to create a meaningful test case.
(0020601)
kgv   
2012-05-29 18:45   
Dear vro, here are my remarks:

General remark - please remove all tabulation symbols from patch.

> Image_PixMap::PixelColorWithAlpha()
the whole method is code duplication.
You may re-implement Image_PixMap::PixelColor() by calling Image_PixMap::PixelColorWithAlpha() with dummy argument.
Moreover there no need for method with such name - use Image_PixMap::PixelColor() for both.

> //Set alpha to 255 (opaque)
> theAlpha = 255;
This is inconvenient to set alpha value to magic 255.0f (look also that you set alpha FIT_RGBAF in different manner).
theAlpha argument should be Quantity_Parameter and within 0..1 range.
(0020602)
kgv   
2012-05-29 19:03   
(edited on: 2012-05-29 19:04)
VGetPixelColorWithAlpha looks very ugly name. Is it really useful to show color name via Quantity_Color::StringName() in output?

Also command is very similar to QAAISGetPixelColor. This maybe useful to merge both commands to provide flexible interface to the same functionality with more convenient name like vpixelcolor.

(0020603)
san   
2012-05-29 19:08   
Please, consider remarks by kgv.
(0020657)
vro   
2012-06-07 15:14   
Dear kgv,
Tabulations are removed.
The method looks like some code duplication because the original method is very important in QA activity of Open CASCADE and I wouldn't like to modify it.
The draw-command is renamed to vpixelcolor. Indeed, it is a better name.
The draw command just tests the imporvement. It means that it is not used anywhere. If somebody uses it and it is not convenient for him to obtain a color by the string name, I agree in advance to modify the draw command anyway the user of the draw command wants.

The modifications are put into CR23156.
(0020668)
kgv   
2012-06-08 11:53   
Dear san and vro,

please review modified patch in branch CR23156_2.
(0020671)
vro   
2012-06-08 12:23   
I agree with the changes.
(0020674)
san   
2012-06-08 13:13   
Branch CR23156_2 reviewed without remarks, ready for testing.
(0020675)
san   
2012-06-08 15:16   
Dear mkv,

The remote branch CR23156_2 has been updated once more.
Please, update your local copy of CR23156_2 if you have already checked it out.
(0020678)
mkv   
2012-06-08 15:45   
Dear san,
My local copy of CR23156_2 has been updated.
(0020695)
mkv   
2012-06-14 17:35   
Dear BugMaster,
The workbenches
KAS:dev:mkv-23156-occt-2 (GIT branch CR23156_2)
KAS:dev:mkv-23156-products-2 (GIT master)
were compiled on Linux platform and tested.

Regression:
Not detected

Improvements:
Not detected

Testing case:
Test case with new draw command (vreadpixel) for this bug is chl 934 E0.
It is OK in this branch.

See results in /QADisk/occttests/results/KAS/dev/mkv-23156-products-2_08062012/lin
See reference results in /QADisk/occttests/results/KAS/dev/products-20120601-opt_02062012/lin
See test cases in /QADisk/occttests/tests/ED
N.B. In order to launch testing case you can make use the following instructions
http://doc/doku.php?id=occt:certification [^]
(0020696)
mkv   
2012-06-14 17:35   
Dear kgv,
1.It nesessary to correct help string for vreadpixel (imho):

Usage : vreadpixel xPixel yPixel [{rgb|rgba|depth|hls|rgbf|rgbaf|name}=rgba]

2. Also, could you please validate the result of test case fort his fix (chl/934/E0)?
(0020710)
kgv   
2012-06-15 17:05   
2mkv,

> 1.It nesessary to correct help string for vreadpixel (imho):
> Usage : vreadpixel xPixel yPixel [{rgb|rgba|depth|hls|rgbf|rgbaf|name}=rgba]
Although this is not very usable but current implementation allows 'name' as additional option (for example depth + name). Maybe you suggest another command syntax?

> 2. Also, could you please validate the result
> of test case fort his fix (chl/934/E0)?
I don't think that unit tests for auxiliary method are meaningful (though command may be used for functional tests in future).
Also testing depth value with transparency (vsettransparency) is almost pointless (depth buffer is disabled -> result always 1.0).
(0020747)
mkv   
2012-06-21 15:29   
1. I have not another command syntax.
2. Me mast have simple test for new functionality.