address conflict between visualization and VTK head

[greenbrettmichael]

error in vtk/rendering/core introduced by Kitware/VTK@bbc96ed

Closes #2623

[greenbrettmichael]

appveyor appears to have timed out during the build process

[taketwo]

Hi, the lines that you are changing have quite a history, please refer to #2291, #2311, and #2322. As you can see, we have been bouncing between different version checks for a while. I would like to avoid a situation that we merge a commit that fixes the problem for you, but breaks builds for others. Please check especially the last thread that I referenced. We need to know a specific released VTK version where the API changed.

[marcel-bariou]

@taketwo @SergioRAgostinho ...Hi, the lines that you are changing have quite a history, please refer to #2291, #2311, and #2322. As you can see, we have been bouncing between different version checks for a while ....

Ubuntu 18.04 LTS
GCC 4.3 VTK 8.2 with PCL 1.9+ Latest commit 9073f63 2018/11/14
I pickup the 2612 change #ii..#else ..#endif, after code line 3563 and I apply it to the previous commit, it works perfectly thanks ! :

 #if VTK_MAJOR_VERSION >= 8 && VTK_MINOR_VERSION >= 2
     mapper->MapDataArrayToMultiTextureAttribute(mesh.tex_materials[tex_id].tex_name.c_str(),
       this_coordinates_name.c_str(),
       vtkDataObject::FIELD_ASSOCIATION_POINTS);
 #else
     mapper->MapDataArrayToMultiTextureAttribute(tu,
                                                 this_coordinates_name.c_str (),
                                                 vtkDataObject::FIELD_ASSOCIATION_POINTS);
     this_coordinates_name.c_str(),
     vtkDataObject::FIELD_ASSOCIATION_POINTS);
 #endif    

    polydata->GetPointData ()->AddArray (coordinates);
    actor->GetProperty ()->SetTexture (tu, texture);
    ++tex_id;
  }

Maintainer edit: aesthetics

[greenbrettmichael]

@taketwo

the API changes mapper->MapDataArrayToMultiTextureAttribute to take a string, whereas SetTexture remains an integer. This prevents the approach given by the previous define from working. This was changed on VTK commit Kitware/VTK@bbc96ed 10 months ago.

This change is encorporated into the upcoming release https://blog.kitware.com/vtk-8-2-0-rc1-ready-for-testing/ . The previous define would have never activated as VTK has not release a version 9, so this change is safe from breaking users with previous versions.

[taketwo]

Thanks for investigating. I just have a minor suggestion then, can we keep the #if/#else stuff at the same place as it is now, only update the version check:

#if (VTK_MAJOR_VERSION == 8 && VTK_MINOR_VERSION >= 2) || VTK_MAJOR_VERSION > 8
  const char *tu = mesh.tex_materials[tex_id].tex_name.c_str ();
#else
  int tu = vtkProperty::VTK_TEXTURE_UNIT_0 + tex_id;
#endif

[marcel-bariou]

Thanks I will look, let me a day Le mar. 20 nov. 2018 à 09:21, Sergey Alexandrov <[email protected]> a écrit :

…

Thanks for investigating. I just have a minor suggestion then, can we keep the #if/#else stuff at the same place as it is now, only update the version check: #if (VTK_MAJOR_VERSION == 8 && VTK_MINOR_VERSION >= 2) || VTK_MAJOR_VERSION > 8 const char *tu = mesh.tex_materials[tex_id].tex_name.c_str (); #else int tu = vtkProperty::VTK_TEXTURE_UNIT_0 + tex_id; #endif — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2612 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGLaSWeOv401v4VmopvlHXY9wrfROno2ks5uw7uKgaJpZM4YYhzA> .

[greenbrettmichael]

@taketwo I agree that is more aesthetically pleasing and nicer to the commit history. However line 3570 requires int tu in all VTK versions
actor->GetProperty ()->SetTexture (tu, texture);
As a compromise, how about:

#if (VTK_MAJOR_VERSION == 8 && VTK_MINOR_VERSION >= 2) || VTK_MAJOR_VERSION > 8
  const char *tu = mesh.tex_materials[tex_id].tex_name.c_str ();
#else
  int tu = vtkProperty::VTK_TEXTURE_UNIT_0 + tex_id;
#endif
int tuID = vtkProperty::VTK_TEXTURE_UNIT_0 + tex_id;
...
mapper->MapDataArrayToMultiTextureAttribute(tu,
  this_coordinates_name.c_str(),
  vtkDataObject::FIELD_ASSOCIATION_POINTS);
polydata->GetPointData ()->AddArray (coordinates);
actor->GetProperty ()->SetTexture (tuID, texture);

I hope that I am not coming off as a pedant, I have more contributions to make!

[taketwo]

Maybe I'm checking in the wrong place, but I see the following signature:

void vtkProperty::SetTexture(const char* name, vtkTexture* tex)

Edit: the commit you referred to earlier has decorated the int signature with VTK_LEGACY.

[greenbrettmichael]

@taketwo I concede that I was mistaken. I have pushed the requested change. This builds for me

[taketwo]

LGTM, thanks.

[SergioRAgostinho]

Thanks everyone.

Edit: I just noticed I merged again instead of squashing...