Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add reflectivity tool to Depth Quality Tool application #7675

Merged
merged 2 commits into from Nov 3, 2020

Conversation

Nir-Az
Copy link
Collaborator

@Nir-Az Nir-Az commented Oct 29, 2020

Adding reflectivity tool to DQT:

  • Add new option "RS2_NOISE_ESTIMATION"
  • Add new option "RS2_ENABLE_IR_REFLECTIVITY"
  • Add reflectivity algorithm class
  • Add display at the DQT application
  • Current calculations for display occurs at frame rate

UI:
image

Note: This PR does not contain all logical conditions for activating IR Reflectivity

Tracked on [RS5-9263]

{
if (value == 1.0f)
{
auto &max_usable_range_option = _l500_depth_dev->get_depth_sensor().get_option(RS2_OPTION_ENABLE_MAX_USABLE_RANGE);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should do this only AFTER we've checked that all the other options are acceptable...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

const char * ir_reflectivity_option::get_description() const
{

return "IR Reflectivity Tool� calculates the percentage of IR light reflected by the "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a symbol after Tool that I don't think should be there

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

{

return "IR Reflectivity Tool� calculates the percentage of IR light reflected by the "
"object and returns to the camera for processing.\n For example, a value of 60% means "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space after \n shouldn't be there

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


return "IR Reflectivity Tool� calculates the percentage of IR light reflected by the "
"object and returns to the camera for processing.\n For example, a value of 60% means "
"that 60% of the IR light projected by L515 is reflected by the object and returns "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by L515 -> by the camera

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

src/types.cpp Outdated
@@ -447,6 +447,8 @@ namespace librealsense
CASE(SEQUENCE_ID)
CASE(HUMIDITY_TEMPERATURE)
CASE(ENABLE_MAX_USABLE_RANGE)
CASE(NOISE_ESTIMATION)
CASE(ENABLE_IR_REFLECTIVITY)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to override this one manually to return Enable IR Reflectivity

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cause we want capitalized R in IR ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

wrappers/matlab/option.m Outdated Show resolved Hide resolved
@@ -182,6 +182,8 @@ PYBIND11_MODULE(NAME, m) {
.value("sequence_id", RS2_OPTION_SEQUENCE_ID)
.value("humidity_temperature", RS2_OPTION_HUMIDITY_TEMPERATURE)
.value("enable_max_usable_range", RS2_OPTION_ENABLE_MAX_USABLE_RANGE)
.value("noise_estimation", RS2_OPTION_NOISE_ESTIMATION)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

common/viewer.h Outdated
@@ -188,12 +192,13 @@ namespace rs2

std::shared_ptr<updates_model> updates;

bool draw_max_usable_range = true;
std::unordered_set<int> _hided_options;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_hidden_options? No such thing as hided :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

common/viewer.h Outdated Show resolved Hide resolved
common/viewer.h Outdated Show resolved Hide resolved
if (_reflectivity_data_collected)
{
viewer.ref.reset();
_reflectivity_data_collected = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we make it so the Reflectivity class knows whether data has been collected (essentially, if it's not empty)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can I thought about it too but wasn't sure what I preffer , if you think its better I can change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

common/model-views.cpp Outdated Show resolved Hide resolved
common/model-views.cpp Outdated Show resolved Hide resolved
viewer.ref.add_input(val, x, y);
_reflectivity_data_collected = true;

float noise_est = ds.get_option(RS2_OPTION_NOISE_ESTIMATION);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remind me -- the noise estimation is now shown in the Viewer? With the temperatures?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will add it to the hidden options for the viewer+ DQT since nobody asked for it.

try
{
pixel_ref = viewer.ref.get_reflectivity(noise_est, max_usable_range, ir_val);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra line

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

common/model-views.cpp Outdated Show resolved Hide resolved
return false;
} );

if (0 == count_valid)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed? Why not just use _filt_dist_arr.empty()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

float reflectivity::get_reflectivity(float raw_noise_estimation,float max_usable_range, float ir_val) const
{
//Calculating STD over time(temporal noise), when there are more than 5 % invalid return high value
std::vector<float> _filt_dist_arr(STD_PERIOD);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not _dist_queue.size()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

using namespace rs2;


static const int STD_PERIOD = 100;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

N_STD_FRAMES? Period implies time...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

return true;
}
return false;
} );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

static const int VGA_HALF_WIDTH = 320;
static const int VGA_HALF_HEIGHT = 240;

static bool is_close_to_zero(float x) { return (std::abs(x) < std::numeric_limits<float>::epsilon()); }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

{
float variance = 0.0f;
for( auto val : _filt_dist_arr )
variance += pow( val - mean, 2.0f );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we do all this why we're counting count_valid? Then maybe we don't need to copy to another vector?

Copy link
Collaborator Author

@Nir-Az Nir-Az Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed as discussed (count_vaid removed)

standard_deviation = sqrt( variance );
}

//Range is calculated based on position in map(assuming 0 tilt) Distance is just based on plane distance
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space between // and the text, please

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}
catch (...) {};
if (pixel_ref != -1.0f)
ref_str = to_string() << std::dec << round(pixel_ref * 100) << "%";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move into the try block

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

auto max_usable_range = mur_sensor.get_max_usable_depth_range();
reflectivity_valid = true;
std::string ref_str = "N/A";
float pixel_ref = -1.0f;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

{
pixel_ref = _reflectivity->get_reflectivity(noise_est, max_usable_range, ir_val);
}
catch (...) {};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

&& ds.supports( RS2_OPTION_ENABLE_MAX_USABLE_RANGE )
&& ( p.stream_type() == RS2_STREAM_INFRARED ) || ( p.stream_type() == RS2_STREAM_DEPTH ) )
{
_reflectivity = std::unique_ptr< reflectivity >(new reflectivity());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we check whether it's skipped here rather than later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

variance += pow( val - mean, 2.0f );

variance = variance / _filt_dist_arr.size();
standard_deviation = sqrt( variance );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a double conversion, no? Maybe use std::sqrt...
Make sure no warnings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

static bool is_close_to_zero(float x) { return (std::abs(x) < std::numeric_limits<float>::epsilon()); }


reflectivity::reflectivity() : _is_empty(false)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@maloel maloel merged commit 7573fee into IntelRealSense:development Nov 3, 2020
@Nir-Az Nir-Az deleted the add_reflectivity_tool branch December 23, 2020 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants