Improve 'u' key in PCLVisualizer: search in shapes LUT too

[VictorLamoine]

Fixes #1240

Features

1st commit

• Remove addModelFromPolyData(vtkPolyDataMapper) because it is not needed (mapper can be set to a vtkPolyData) and causes some visualization functions to crash (setting shading for example)

2nd commit

• The user can choose which cloud/shape LUT should be displayed with setLookUpTableID
• LUT display is updated if the user triggers a setShapeProperties. The LUT is only updated if the shape updated is the one used for the LUT display (either because it is the one found automatically or the user defined which ID should be used)
• The LUT is removed from the display if no cloud/shape was found with color information or if the user provided a wrong (= non existing or no color info) cloud/shape ID.

Examples

[taketwo]

I wonder how you plan to implement the third item :)

[VictorLamoine]

I don't think that will be ever possible!
And the first one probably implies big changes in the PCLVisualizer

[VictorLamoine]

The code needs a lot of cleaning/refactoring but I think the idea is there.
I'm getting more and more keen on VTK, what a great library!

[taketwo]

Smart!

[taketwo]

So you are now VTK and LUT expert, Victor... maybe you can address #1158 as well?

[VictorLamoine]

Because of #1244 I'm stuck.

[VictorLamoine]

Pull request/first message updated, please test with the example code provided!
I do not solve #1158 because it looks like a lot of work, but this pull request is clearly a step forward to solving it.

[taketwo]

I don't understand how your code makes sure that the LUT for the last cloud/shape added to the rendered is displayed. Can you explain the logic? Specifically, why you use forward iterators and break (not continue) inside the loops.

[VictorLamoine]

It's an error, it should be continue!

If no cloud/shape id is specified, we'll search for any cloud/shape that has a scalars to display the lookup table. If we found none, we should not display any lookup table. (found_actor = false)

When we exit the loop, if actor_found is set to true, we have found (at least) one actor with a lookup table so we will display this one, it will be either updated or added depending on lut_enabled_ status.

I'm still updating this, it's not ready for merging but I would like this to be reviewed by other people.

[taketwo]

But since you want the last one added, shouldn't you rather iterate from the end to the beginning?

I'm still updating this, it's not ready for merging but I would like this to be reviewed by other people.

To make the status of a pull request clear we may adopt a wide-spread convention: prefix the name with [WIP] if you are still working on the request, or [RFC] if you think you are done, but want comments.

[VictorLamoine]

But since you want the last one added, shouldn't you rather iterate from the end to the beginning?

I'm not sure how I can achieve this, the ShapeActor typedef is a boost::underdored_map

typedef boost::unordered_map<std::string, vtkSmartPointer<vtkProp> > ShapeActorMap;

As is; the LUT displayed is the last found, it does not especially correspond to the last shape added.

[VictorLamoine]

@jspricke and @taketwo review would be appreciated 🐙

From what I've tested it works ok!

[taketwo]

I'm on vacation till the end of the month. Will have a look early next week.

[taketwo]

Why public?

[VictorLamoine]

I thought that a getter/setter would add useless complexity so I made it public.
The PCLVisualizer directly modifies the string here for example.

[taketwo]

PCLVisualizer is a friend, so will be able access this even if it is made private. I would strongly prefer to not have public member fields.

[taketwo]

I'm not sure how I can achieve this, the ShapeActor typedef is a boost::underdored_map

You are right, sorry. Unordered map does not have a reverse iterator.

[VictorLamoine]

@taketwo Some free time for reviewing?

[taketwo]

For me this last section is a bit hard to follow, e.g. it's not immediately obvious that all cases/possibilities are covered. But since I don't have any specific suggestions how it could be improved, and since it actually works (until proved otherwise :) ), I think I'm fine with merging this pull request. Please make a fix according to my inline comment though. Thanks!

[VictorLamoine]

lut_actor_id_ is now private.

Yes this section is quite tricky and handles all the cases well from what I've tested.
I will write a tutorial about this later.