View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0023156CommunityOCCT:Visualizationpublic2012-05-15 10:362012-11-16 13:16
Reportervro Assigned Tosan 
PrioritynormalSeverityminor 
Status closedResolutionfixed 
PlatformWindowsOSVC++ 2005 
Product Version6.5.3 
Target Version6.5.4Fixed in Version6.5.4 
Summary0023156: A class Image_PixMap doesn't give direct access to transparency information
DescriptionA 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.
TagsNo tags attached.
Test case numberchl 934 E0

Attached Files

  • E0 (7,708 bytes)

Activities

vro

2012-05-15 10:51

developer   ~0020534

A Git branch CR23156 contains a new method Image_PixMap::PixelColorWithAlpha().
Could you review the patch please?

san

2012-05-22 19:08

developer   ~0020566

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.

vro

2012-05-23 10:30

developer   ~0020567

Fixed. Thank you for so careful review!

san

2012-05-23 13:34

developer   ~0020569

What about the DRAW command for testing new Image_PixMap method?
Without it the test case cannot be created for this issue.

vro

2012-05-29 14:50

developer   ~0020599

A draw-command VGetPixelColorWithAlpha is added.

san

2012-05-29 17:57

developer   ~0020600

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.

kgv

2012-05-29 18:45

developer   ~0020601

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.

kgv

2012-05-29 19:03

developer   ~0020602

Last edited: 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.

san

2012-05-29 19:08

developer   ~0020603

Please, consider remarks by kgv.

vro

2012-06-07 15:14

developer   ~0020657

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.

kgv

2012-06-08 11:53

developer   ~0020668

Dear san and vro,

please review modified patch in branch CR23156_2.

vro

2012-06-08 12:23

developer   ~0020671

I agree with the changes.

san

2012-06-08 13:13

developer   ~0020674

Branch CR23156_2 reviewed without remarks, ready for testing.

san

2012-06-08 15:16

developer   ~0020675

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.

mkv

2012-06-08 15:45

tester   ~0020678

Dear san,
My local copy of CR23156_2 has been updated.

mkv

2012-06-14 17:35

tester   ~0020695

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

mkv

2012-06-14 17:35

tester   ~0020696

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)?

mkv

2012-06-14 17:36

tester  

E0 (7,708 bytes)

kgv

2012-06-15 17:05

developer   ~0020710

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

mkv

2012-06-21 15:29

tester   ~0020747

1. I have not another command syntax.
2. Me mast have simple test for new functionality.

Related Changesets

occt: master 85e096c3

2012-06-22 07:29:55

vro


Committer: san Details Diff		 0023156: Image_PixMap::PixelColor() extended to return alpha value

Implemented new method to provide access to alpha value in image.

Implemented new vreadpixel Draw Harness command to read
specified pixel value from 3D view.
vdump command - added result checks
Corrected misprint		 Affected Issues
0023156
mod - src/Image/Image_PixMap.cdl Diff File
mod - src/Image/Image_PixMap.cxx Diff File
mod - src/V3d/V3d_View.cxx Diff File
mod - src/ViewerTest/ViewerTest.cxx Diff File
mod - src/ViewerTest/ViewerTest_ViewerCommands.cxx Diff File

Issue History

Date Modified Username Field Change
2012-05-15 10:36 vro New Issue
2012-05-15 10:36 vro Assigned To => vro
2012-05-15 10:50 vro Assigned To vro => san
2012-05-15 10:51 vro Note Added: 0020534
2012-05-15 10:51 vro Status new => resolved
2012-05-22 19:08 san Note Added: 0020566
2012-05-22 19:08 san Assigned To san => vro
2012-05-22 19:08 san Status resolved => assigned
2012-05-23 10:30 vro Note Added: 0020567
2012-05-23 10:30 vro Assigned To vro => san
2012-05-23 10:30 vro Status assigned => resolved
2012-05-23 13:34 san Note Added: 0020569
2012-05-23 13:34 san Assigned To san => vro
2012-05-23 13:34 san Status resolved => assigned
2012-05-29 14:50 vro Note Added: 0020599
2012-05-29 14:50 vro Assigned To vro => san
2012-05-29 14:50 vro Status assigned => resolved
2012-05-29 17:57 san Note Added: 0020600
2012-05-29 17:57 san Assigned To san => bugmaster
2012-05-29 17:57 san Status resolved => reviewed
2012-05-29 18:26 mkv Assigned To bugmaster => aan
2012-05-29 18:45 kgv Note Added: 0020601
2012-05-29 19:03 kgv Note Added: 0020602
2012-05-29 19:03 kgv Note Edited: 0020602
2012-05-29 19:04 kgv Note Edited: 0020602
2012-05-29 19:08 san Note Added: 0020603
2012-05-29 19:08 san Assigned To aan => vro
2012-05-29 19:08 san Status reviewed => assigned
2012-06-07 15:14 vro Note Added: 0020657
2012-06-07 15:14 vro Assigned To vro => kgv
2012-06-07 15:14 vro Status assigned => resolved
2012-06-08 11:51 kgv Assigned To kgv => san
2012-06-08 11:53 kgv Note Added: 0020668
2012-06-08 12:23 vro Note Added: 0020671
2012-06-08 13:13 san Note Added: 0020674
2012-06-08 13:13 san Assigned To san => bugmaster
2012-06-08 13:13 san Status resolved => reviewed
2012-06-08 14:24 mkv Assigned To bugmaster => mkv
2012-06-08 15:16 san Note Added: 0020675
2012-06-08 15:45 mkv Note Added: 0020678
2012-06-14 17:35 mkv Note Added: 0020695
2012-06-14 17:35 mkv Note Added: 0020696
2012-06-14 17:36 mkv File Added: E0
2012-06-14 17:37 mkv Test case number => chl 934 E0
2012-06-14 17:37 mkv Assigned To mkv => kgv
2012-06-14 17:37 mkv Status reviewed => assigned
2012-06-15 17:05 kgv Note Added: 0020710
2012-06-19 22:07 kgv Assigned To kgv => mkv
2012-06-21 15:29 mkv Note Added: 0020747
2012-06-21 15:29 mkv Assigned To mkv => bugmaster
2012-06-21 15:30 mkv Status assigned => resolved
2012-06-21 15:30 mkv Status resolved => reviewed
2012-06-21 15:30 mkv Status reviewed => tested
2012-06-25 20:05 san Changeset attached => occt master 85e096c3
2012-06-25 20:05 san Assigned To bugmaster => san
2012-06-25 20:05 san Status tested => verified
2012-06-25 20:05 san Resolution open => fixed
2012-11-16 13:15 bugmaster Fixed in Version => 6.5.4
2012-11-16 13:16 bugmaster Status verified => closed