search

wanadev / gltf-bounding-box

Computes the global bounding box of a gltf model
Other
20 stars 7 forks source link

Added precision to bounding box with custom round function #6

Closed riccardogiorato closed 5 years ago

riccardogiorato commented 6 years ago

Added precision to bounding box with custom round function.

This enables to get accurate sizes for objects.

Current users won't notice the change cause with default param the module API will act the same.

risq commented 6 years ago

Thank you for your PR !!

Could you please add a few unit tests to test the precision option with the example model ?

Thanks.

riccardogiorato commented 6 years ago

I added dist build cause I wanted to use them as soon as possible on my projects. About tests, I tried fixing them but it kept failing with NaN results. Could it be caused by spy?

risq commented 6 years ago

Indeed, I have investigated a little and I found out that the main function was passing a buffer object to gltf1 & glb functions as a second parameter, which was used as the precision in the computeBoundings methods.

I just pushed a fix on master, tell me if you still have issues with your tests.

riccardogiorato commented 6 years ago

Your commit is still failing the build: "The Travis CI build failed"

risq commented 6 years ago

OK, it is fixed now, it was an issue between travis and some of the dependencies of gulp.

riccardogiorato commented 5 years ago

Can you merge this pull request? It might be really usefuel to have the variable precision for 3d models

aronfiechter commented 5 years ago

Will this PR be merged? I'm not sure what the issue is with the failing build, but I agree with @Giorat that a precision parameter would be really helpful.

aronfiechter commented 5 years ago

I forked the fork and corrected some issues, should I create a pull request with (my changes) to this repo or to @Giorat's fork?

riccardogiorato commented 5 years ago

To mine so then I can merge into this one! 😄

aronfiechter commented 5 years ago

@Giorat done

riccardogiorato commented 5 years ago

@Giorat done

Still present the issue

aronfiechter commented 5 years ago

Still present the issue

Yes, there are still merge conflicts, but we cannot do anything about those. But the tests are now all passing

@risq what are the conflicts in index.js about?

aronfiechter commented 5 years ago

@risq the build fails likely because of a problem with gulp and node 11: https://github.com/gulpjs/gulp/issues/2246