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

Support VTK 9.x #36

Closed
kwabenantim opened this issue Dec 13, 2022 · 7 comments · Fixed by #152
Closed

Support VTK 9.x #36

kwabenantim opened this issue Dec 13, 2022 · 7 comments · Fixed by #152

Comments

@kwabenantim
Copy link
Member

This issue continues the discussion for legacy trac ticket 3088:


fcooper8472 created the following ticket on 2022-02-02 at 13:26:24, it is owned by fcooper8472

Changes can be seen here: https://www.kitware.com/vtk-9-0-0-available-for-download/

This currently prevents Chaste working (easily) on macOS as Homebrew gives v9.


Comment by jmpf on 2022-08-01 at 11:05:05

VTK9 was added to the rotation in February:

but it has yet to compile successfully: https://chaste.cs.ox.ac.uk/buildbot/builders/Portability%206_Sat?numbuilds=30


Comment by jmpf on 2022-08-04 at 13:33:48

Core components that need VTK are building fine (global, mesh, etc.).

The cell_based component fails when including VtkMeshReader.hpp. This is probably because it doesn't have the correct include path, but I can't work out why that may be.


Comment by jmpf on 2022-10-24 at 15:17:59

Replying to jmpf@…:

Core components that need VTK are building fine (global, mesh, etc.).

The cell_based component fails when including VtkMeshReader.hpp. This is probably because it doesn't have the correct include path, but I can't work out why that may be.

Note that this is a CMake configuration problem and is to do with the way CMake now detects libraries and include paths.

@kwabenantim kwabenantim added this to the Iteration I5 milestone Dec 13, 2022
@AlexFletcher AlexFletcher changed the title #3088 - Support VTK 9.0 Support VTK 9.0 Jul 4, 2023
@fcooper8472 fcooper8472 linked a pull request Aug 9, 2023 that will close this issue
@fcooper8472 fcooper8472 changed the title Support VTK 9.0 Support VTK 9.x Aug 10, 2023
@fcooper8472
Copy link
Member

I have tested #152 against VTK 9.1, and the entire continuous test pack compiles and runs fine.

VTK 9.2 is going to be slightly more involved. One method was deprecated, which is addressed in 6218397.

However, VTK 9.2 causes TestVtkMeshReader to fail:

Entering TestGetNextFaceData

/home/fcooper/GitRepos/Chaste/Chaste/mesh/test/reader/TestVtkMeshReader.hpp:206: Error: Expected (first_face_data.NodeIndices[0] == 11u), found (0 != 11)
/home/fcooper/GitRepos/Chaste/Chaste/mesh/test/reader/TestVtkMeshReader.hpp:207: Error: Expected (first_face_data.NodeIndices[1] == 3u), found (11 != 3)
/home/fcooper/GitRepos/Chaste/Chaste/mesh/test/reader/TestVtkMeshReader.hpp:208: Error: Expected (first_face_data.NodeIndices[2] == 0u), found (3 != 0)
/home/fcooper/GitRepos/Chaste/Chaste/mesh/test/reader/TestVtkMeshReader.hpp:211: Error: Expected (next_face_data.NodeIndices[0] == 3u), found (0 != 3)
/home/fcooper/GitRepos/Chaste/Chaste/mesh/test/reader/TestVtkMeshReader.hpp:212: Error: Expected (next_face_data.NodeIndices[1] == 8u), found (3 != 8)
/home/fcooper/GitRepos/Chaste/Chaste/mesh/test/reader/TestVtkMeshReader.hpp:213: Error: Expected (next_face_data.NodeIndices[2] == 0u), found (8 != 0)
/home/fcooper/GitRepos/Chaste/Chaste/mesh/test/reader/TestVtkMeshReader.hpp:218: Error: Expected (first_face_data.NodeIndices[0] == 11u), found (0 != 11)
/home/fcooper/GitRepos/Chaste/Chaste/mesh/test/reader/TestVtkMeshReader.hpp:219: Error: Expected (first_face_data.NodeIndices[1] == 3u), found (11 != 3)
/home/fcooper/GitRepos/Chaste/Chaste/mesh/test/reader/TestVtkMeshReader.hpp:220: Error: Expected (first_face_data.NodeIndices[2] == 0u), found (3 != 0)
/home/fcooper/GitRepos/Chaste/Chaste/mesh/test/reader/TestVtkMeshReader.hpp:223: Error: Expected (next_face_data.NodeIndices[0] == 3u), found (0 != 3)
/home/fcooper/GitRepos/Chaste/Chaste/mesh/test/reader/TestVtkMeshReader.hpp:224: Error: Expected (next_face_data.NodeIndices[1] == 8u), found (3 != 8)
/home/fcooper/GitRepos/Chaste/Chaste/mesh/test/reader/TestVtkMeshReader.hpp:225: Error: Expected (next_face_data.NodeIndices[2] == 0u), found (8 != 0)
Failed

It looks as though the indices might be being re-ordered? Needs further investigation.

@jmpf
Copy link
Contributor

jmpf commented Aug 11, 2023

There are tests above that one for

  • node order/position
  • element order
    These both work as expected and Chaste meshes use nodes and elements as the primary data structures.

Here the order of nodes within faces have been given an even permutation. This does not affect chirality so it's something that changes nothing. It's possible that new (or old) VTK gives back the other chirality and we flip it in the constructor.

I suggest putting the indices into std::set and using set equality with a comment to say that 9.2 is giving a different ordering. (Possibly overkill, but we're only doing it a couple of times.)

@fcooper8472
Copy link
Member

@jmpf just to make sure I've understood correctly, anything up to a rotation is correct, so {11, 3, 0}, {3, 0, 11} and {0, 11, 3} are all "correct"?

Perhaps there's an elegant way of checking for a rotation...

@jmpf
Copy link
Contributor

jmpf commented Aug 11, 2023

@fcooper8472 Yes. These all appear to be correct. The other way to do the test is to #ifdef on the VTK version...

I changed by mind about auto-flipping chirality in the constructor -- we can't do that for a triangular face in 3D.

@fcooper8472
Copy link
Member

How about something like this?

c4d048d

@jmpf
Copy link
Contributor

jmpf commented Aug 11, 2023

Probably overkill, but the rationale of the test is now crystal-clear.

@fcooper8472
Copy link
Member

VTK 9.0, 9.1 and 9.2 appear to all be working.

This issue can be closed as soon as the portability workflow is updated to include the new portability-06 image.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants