Conversation
gaschler
left a comment
gaschler
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
I also think reusing a mesh import lib is a good choice here and makes things simpler.
I added a couple of comments to the code, please resolve.
Feel free to ask if any comment doesn't make sense.
Just a hint for the future, before you write such large patch, feel free to ask
if the feature is needed, not all pull requests will be merged, especially the kind of patches that add dependencies.
Thanks again, looking forward to seeing this merged
| return; | ||
| for (int meshIndex = 0; meshIndex < scene->mNumMeshes; meshIndex ++ ){ | ||
| const aiMesh* myMesh = scene->mMeshes[meshIndex]; | ||
| for (int i = 0; i < myMesh->mNumVertices; i++){ |
There was a problem hiding this comment.
same here, for (const auto& vertex : mesh->mVertices)
also below for the faces
| // And have it read the given file with some example postprocessing | ||
| // Usually - if speed is not the most important aspect for you - you'll | ||
| // propably to request more postprocessing than we do in this example. | ||
| const aiScene* scene = importer.ReadFile( pFile, |
There was a problem hiding this comment.
This is quite a change.
Please confirm you are actually planning fix bugs if this introduces any.
| @@ -4,9 +4,16 @@ find_package(Eigen 3 REQUIRED) | |||
| find_package(Coin) | |||
There was a problem hiding this comment.
Because this a quite a change, run all tests with docker.
Please install docker and follow the command documented in https://github.com/gaschler/bounding-mesh#contribute
| find_package(Coin) | ||
| find_package(CGAL) | ||
| find_package(GMP) | ||
| find_package(assimp) |
There was a problem hiding this comment.
Not sure if boundingmesh is used with Windows by anyone.
How likely will Windows users be able to get this compiled with this new dependency?
There was a problem hiding this comment.
Normally assimp should work fine with windows
There was a problem hiding this comment.
But I don't really know for the installation part
|
Feel free to ask if there are any questions how to get this fixed and merged. |
|
I did not have much time lately, I plan to fix everything this week ! |
antoninklopp
force-pushed
the
load-with-assimp
branch
from
925410b to
c5455e0
Compare
|
I still need to fix the docker build with ninja not finding assimp at step 7. |
2 participants
Hi,
Thanks for this amazing project, which is working really great.
However, there are a lot of file types not supported and even for file types supported, some are not loading as expected. I think that you could benefit from using a library like assimp to load your objects.
It simplifies the code and allows a better loading.
I did it with a simple car :
With your loading
With assimp loading
Some artefacts remain on the car dur to the fact that the obj file defines several parts and your object is a single array
It comes from the fact that some .obj files can have 4 vertices coordinates and you are only reading 3 for example, and the result is that you are missing one triangle over two.
It is also possible to load files like .glb, this one is working great for example avocado. However some are not working (buggy) because of asserts in the addTriangle method.
Hope this helps !
Thanks again for this amazing project.