Remove depreciated VTK function MapDataArrayToMultiTextureAttribute #2291

jasjuang · 2018-04-28T02:39:24Z

VTK removed MapDataArrayToMultiTextureAttribute in commit https://gitlab.kitware.com/vtk/vtk/commit/2f231fbd33fd936c23d5b4a375e6a2c542b771d3. This PR uses the newer functions to achieve the same functionality.

taketwo · 2018-04-28T21:32:46Z

Unfortunately this is incompatible with VTK 5. I know it's super old, but it's still used on Travis.

What about this:

#if new VTK
  const char* tu = mesh.tex_materials[tex_id].tex_name.c_str ();
#else
  int tu = VTK_TEXTURE_UNIT_0 + tex_id;
#endif

mapper->MapDataArrayToMultiTextureAttribute(tu,
                                            this_coordinates_name.c_str (),
                                            this_coordinates_name.c_str (),
                                            vtkDataObject::FIELD_ASSOCIATION_POINTS);
                                            vtkDataObject::FIELD_ASSOCIATION_POINTS);
polydata->GetPointData ()->AddArray (coordinates);
polydata->GetPointData ()->AddArray (coordinates);
actor->GetProperty ()->SetTexture (tu, texture);
++tex_id;

jasjuang · 2018-04-29T16:13:10Z

Yeah we can try to add macros to support VTK 5. However, new VTK doesn't seem to be working. Do you know whether VTK provide something out of the box or do we have to define something in the CMakeLists ourselves?

taketwo · 2018-04-29T16:51:54Z

Sorry, I did not make it clear, but #if new VTK was pseudo-code (I was lazy to look up what exactly is the name of their version macro). Now I checked, it should be #if VTK_MAJOR_VERSION > 5.

jasjuang · 2018-04-29T17:21:24Z

I added VTK_MAJOR_VERSION to support VTK 5, it's a bit ugly but should do the job.

taketwo · 2018-04-29T17:38:56Z

Please have another look at what I proposed, it is considerably less ugly. The idea is to have a local variable tu, which will have different type (int or string) depending on VTK version. The remaining code stays the same.

jasjuang · 2018-05-02T00:59:47Z

@taketwo I actually asked on VTK about how to solve this, and here is his reponse. Kitware/VTK@2f231fb I just force pushed a commit with the code he pasted. Could you take a look and see whether this make sense to you? If you think it is good I will try to add the VTK 5 support onto it.

taketwo · 2018-05-02T01:47:29Z

I'm fine with the suggestion to drop the "no multi-texturing support" branch. Besides from that, the only thing that is different to your previous attempt is that you now use pcltexture + tex_id instead of mesh.tex_materials[tex_id].tex_name as the texture name. In my opinion, the latter is more meaningful, so I don't see a good reason for this change.

jasjuang · 2018-05-09T02:32:56Z

@taketwo Please take a look and see if this is the preferred change. Thanks.

taketwo · 2018-05-09T02:42:12Z

visualization/src/pcl_visualizer.cpp

  {
    if ((size_t) texture_units < mesh.tex_materials.size ())
      PCL_WARN ("[PCLVisualizer::addTextureMesh] GPU texture units %d < mesh textures %d!\n",
                texture_units, mesh.tex_materials.size ());
    // Load textures
    std::size_t last_tex_id = std::min (static_cast<int> (mesh.tex_materials.size ()), texture_units);
+#if VTK_MAJOR_VERSION > 5
+    std::string tu = mesh.tex_materials[0].tex_name.c_str ();


Shouldn't this be const char*? Because this the type that c_str() returns and also the type that VTK expects? Apart from that looks good.

jasjuang · 2018-05-09T16:30:15Z

@taketwo I just fixed the mistake. Please take a look, thanks!

taketwo · 2018-05-09T16:44:07Z

Why do you create the tu variable outside of the while loop? Please refer to my very first comment, it can be created inside the loop and thus there will be no need for the second ugly #if #else #end block, which makes code more readable.

Also, I've just realized that the if ((mesh.tex_materials.size () > 1)) condition should be removed. Otherwise, we are breaking visualization of meshes with a single material!

…TK newer than 5

jasjuang · 2018-05-14T22:59:39Z

@taketwo You are right. I just forced push a commit with your suggestion. Please take a look.

taketwo

LGTM, thanks

frozar · 2018-05-16T10:32:37Z

Hi there, maybe I'm the only one but on my laptop, after this PR the PCL project doesn't compile anymore.

I got this error:

/home/frozar/wk/pcl_repo/visualization/src/pcl_visualizer.cpp: In member function ‘bool pcl::visualization::PCLVisualizer::addTextureMesh(const pcl::TextureMesh&, const string&, int)’:
/home/frozar/wk/pcl_repo/visualization/src/pcl_visualizer.cpp:3559:88: error: invalid conversion from ‘const char*’ to ‘int’ [-fpermissive]
                                                 vtkDataObject::FIELD_ASSOCIATION_POINTS);

I check the prototype of the function in the header /usr/include/vtk-6.2/vtkPolyDataMapper.h:

  virtual void MapDataArrayToMultiTextureAttribute(
    int unit,
    const char* dataArrayName, int fieldAssociation, int componentno = -1);

And in the file .../visualization/src/pcl_visualizer.cpp you can find:

#if VTK_MAJOR_VERSION > 5
    const char *tu = mesh.tex_materials[tex_id].tex_name.c_str ();
[...]
    mapper->MapDataArrayToMultiTextureAttribute(tu,
                                                this_coordinates_name.c_str (),
                                                vtkDataObject::FIELD_ASSOCIATION_POINTS);

The type of the tu variable doesn't seem to be good, am I wrong? (I use VTK 6.2 which is available in Ubuntu 16.04 package repository.)

taketwo · 2018-05-16T12:51:01Z

Let's make the version check tighter then, #if VTK_MAJOR_VERSION > 7. Would you mind submitting a PR?

frozar · 2018-05-16T13:13:12Z

I'm pretty far of my laptop these days... Frozar Le mer. 16 mai 2018 14:51, Sergey Alexandrov <notifications@github.com> a écrit :

…

Let's make the version check tighter then, #if VTK_MAJOR_VERSION > 8. Would you mind submitting a PR? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2291 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA-k3JiveEvli8B-moRZtg3m_P0E-rPBks5tzCDCgaJpZM4TrNVL> .

taketwo · 2018-05-16T13:14:19Z

How did you discover the problem?!

frozar · 2018-05-16T13:20:15Z

A few hours ago, I have to recompile my project quickly, and I found an Issue (I'm not a lucky man). I need to generate some result but currently I'm in holiday... Le mer. 16 mai 2018 15:14, Sergey Alexandrov <notifications@github.com> a écrit :

…

How did you discover the problem?! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2291 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA-k3Byp9SM9fMBp4BuqhROZATqmJOGlks5tzCY4gaJpZM4TrNVL> .

taketwo · 2018-05-16T13:23:56Z

I see. Well, we have the policy of not merging own pull requests, so I need one of you guys (@jasjuang @frozar) to send a patch, because @SergioRAgostinho is on a... sabbatical.

SergioRAgostinho · 2018-05-16T13:30:31Z

No worries. I’ll look at it later today. Just ping me directly whenever you need something more urgent.

…

--

On Wednesday, May 16, 2018 at 2:24 PM, Sergey Alexandrov ***@***.*** ***@***.***)> wrote: I see. Well, we have the policy of not merging own pull requests, so I need one of you guys ***@***.*** (https://github.com/jasjuang) @frozar (https://github.com/frozar)) to send a patch, because @SergioRAgostinho (https://github.com/SergioRAgostinho) is on a... sabbatical. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub (#2291 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/ABoUVssAPWmhjhEpX8WM55QRQAqgdJZwks5tzCh5gaJpZM4TrNVL).

Fixes an issue introduced by PointCloudLibrary#2291.

taketwo · 2018-05-16T13:39:26Z

Great, thanks. I've sent a PR.

fix missing vtkTexture.h, follow up on #2291

Fixes an issue introduced by PointCloudLibrary#2291 and not completely fixed with PointCloudLibrary#2311

jasjuang force-pushed the master branch from dd41edd to 893b3e9 Compare May 2, 2018 00:56

jasjuang force-pushed the master branch from 893b3e9 to 10bfd9e Compare May 9, 2018 02:32

taketwo reviewed May 9, 2018

View reviewed changes

jasjuang force-pushed the master branch from 10bfd9e to 15916a5 Compare May 9, 2018 02:50

remove depreciated function MapDataArrayToMultiTextureAttribute for V…

ba5a05e

…TK newer than 5

jasjuang force-pushed the master branch from 15916a5 to ba5a05e Compare May 14, 2018 22:58

taketwo approved these changes May 16, 2018

View reviewed changes

taketwo added the module: visualization label May 16, 2018

taketwo merged commit fb44a10 into PointCloudLibrary:master May 16, 2018

taketwo added a commit to taketwo/pcl that referenced this pull request May 16, 2018

Change VTK version check in addTextureMesh

3e08fb7

Fixes an issue introduced by PointCloudLibrary#2291.

taketwo mentioned this pull request May 16, 2018

Change VTK version check in addTextureMesh #2311

Merged

jasjuang mentioned this pull request May 18, 2018

Include missing vtkTexture.h. Follow up on #2291 #2316

Merged

SergioRAgostinho added a commit that referenced this pull request May 20, 2018

Merge pull request #2316 from jasjuang/master

d1c9870

fix missing vtkTexture.h, follow up on #2291

Tones29 added a commit to Tones29/pcl that referenced this pull request May 27, 2018

Second change VTK version check in addTextureMesh

e1796cf

Fixes an issue introduced by PointCloudLibrary#2291 and not completely fixed with PointCloudLibrary#2311

Tones29 mentioned this pull request May 27, 2018

Second change VTK version check in addTextureMesh #2322

Merged

SergioRAgostinho mentioned this pull request Aug 6, 2018

Change Log automation #2385

Closed

SergioRAgostinho mentioned this pull request Aug 25, 2018

Change Log generation tool. Automates change log generation. #2396

Merged

taketwo mentioned this pull request Nov 12, 2018

address conflict between visualization and VTK head #2612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove depreciated VTK function MapDataArrayToMultiTextureAttribute #2291

Remove depreciated VTK function MapDataArrayToMultiTextureAttribute #2291

jasjuang commented Apr 28, 2018

taketwo commented Apr 28, 2018

jasjuang commented Apr 29, 2018

taketwo commented Apr 29, 2018

jasjuang commented Apr 29, 2018

taketwo commented Apr 29, 2018

jasjuang commented May 2, 2018

taketwo commented May 2, 2018

jasjuang commented May 9, 2018

taketwo May 9, 2018

jasjuang commented May 9, 2018

taketwo commented May 9, 2018

jasjuang commented May 14, 2018

taketwo left a comment

frozar commented May 16, 2018

taketwo commented May 16, 2018 •

edited

Loading

frozar commented May 16, 2018 via email

taketwo commented May 16, 2018

frozar commented May 16, 2018 via email

taketwo commented May 16, 2018

SergioRAgostinho commented May 16, 2018 via email

taketwo commented May 16, 2018

Remove depreciated VTK function MapDataArrayToMultiTextureAttribute #2291

Remove depreciated VTK function MapDataArrayToMultiTextureAttribute #2291

Conversation

jasjuang commented Apr 28, 2018

taketwo commented Apr 28, 2018

jasjuang commented Apr 29, 2018

taketwo commented Apr 29, 2018

jasjuang commented Apr 29, 2018

taketwo commented Apr 29, 2018

jasjuang commented May 2, 2018

taketwo commented May 2, 2018

jasjuang commented May 9, 2018

taketwo May 9, 2018

Choose a reason for hiding this comment

jasjuang commented May 9, 2018

taketwo commented May 9, 2018

jasjuang commented May 14, 2018

taketwo left a comment

Choose a reason for hiding this comment

frozar commented May 16, 2018

taketwo commented May 16, 2018 • edited Loading

frozar commented May 16, 2018 via email

taketwo commented May 16, 2018

frozar commented May 16, 2018 via email

taketwo commented May 16, 2018

SergioRAgostinho commented May 16, 2018 via email

taketwo commented May 16, 2018

taketwo commented May 16, 2018 •

edited

Loading