MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Related Changesets ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0023156Community[OCCT] OCCT:Visualizationpublic2012-05-15 10:362012-11-16 13:16
Reportervro 
Assigned Tosan 
PrioritynormalSeverityminor 
StatusclosedResolutionfixed 
PlatformWindowsOSVC++ 2005OS Version32 bit
Product Version[OCCT] 6.5.3 
Target Version[OCCT] 6.5.4Fixed in Version[OCCT] 6.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? file icon E0 (7,708 bytes) 2012-06-14 17:36

- Relationships

-  Notes
(0020534)
vro (developer)
2012-05-15 10:51

A Git branch CR23156 contains a new method Image_PixMap::PixelColorWithAlpha().
Could you review the patch please?
(0020566)
san (developer)
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 (developer)
2012-05-23 10:30

Fixed. Thank you for so careful review!
(0020569)
san (developer)
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 (developer)
2012-05-29 14:50

A draw-command VGetPixelColorWithAlpha is added.
(0020600)
san (developer)
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 (developer)
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 (developer)
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 (developer)
2012-05-29 19:08

Please, consider remarks by kgv.
(0020657)
vro (developer)
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 (developer)
2012-06-08 11:53

Dear san and vro,

please review modified patch in branch CR23156_2.
(0020671)
vro (developer)
2012-06-08 12:23

I agree with the changes.
(0020674)
san (developer)
2012-06-08 13:13

Branch CR23156_2 reviewed without remarks, ready for testing.
(0020675)
san (developer)
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 (tester)
2012-06-08 15:45

Dear san,
My local copy of CR23156_2 has been updated.
(0020695)
mkv (tester)
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 (tester)
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 (developer)
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 (tester)
2012-06-21 15:29

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

- Related Changesets
occt: master 85e096c3
Timestamp: 2012-06-22 07:29:55
Author: 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
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 View Revisions
2012-05-29 19:04 kgv Note Edited: 0020602 View Revisions
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


Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker