cityjson / cityjson-threejs-loader

Apache License 2.0
16 stars 9 forks source link

Geometries get deformed because translation is applied too early #9

Open wlinna opened 1 month ago

wlinna commented 1 month ago

cityjson-loader-broken-translation-handling

This is practically the same issue that cjio has. Geometries are deformed because the whole transform is applied very early in the process. This leads to floating point inaccuracies.

To reproduce the problem, load files in CityJSON Ninja (which uses cityjson-threejs-loader), and notice the difference. The test files are below

simplified2.city.json simplified2_less_translation.city.json

My suggested solution is to only apply the scale from the transform at first, do all the processing normally, and then add the translation afterwards to the position of loader.scene (or if there's a better root node, then that).

StylianosVitalis-TomTom commented 1 month ago

Hey @wlinna,

Thanks for reporting the issue 🙌

I think this is already fixed by this: https://github.com/cityjson/cityjson-threejs-loader/commit/d6a98376424764e4d13388fe23f2adb416cf31c9. It's on the develop branch. I need to merge it to main soon.

I still haven't released a version of ninja that contains this fix as I had some issue with building it and I need to find some time to resolve it. If you are using this library directly, you may think about switching to the develop branch. If your concern is about how this propagates to ninja, I'll do my best to release a new version of it soon so that the fix is included there.

wlinna commented 1 month ago

Hello, thanks for the quick response. I don't use ninja directly, I'm mainly concerned how my CityJSON looks/works elsewhere.

I was looking at that commit, and something that I found interesting is that it doesn't seem to care about the difference of Y and Z conventions between CityJSON and ThreeJS. I would have thought that the scales would be applied in the order of s[0], s[2], s[1], and then similarly for translations, but with the minus sign applied i.e t[0], t[2], -t[1]. Or is the final transformation to ThreeJS's Y-up world handled elsewhere?

StylianosVitalis-TomTom commented 1 month ago

In this library, I do not transform the coordinates to Y-up. You can either use a transformation matrix to finally transform everything to Y-up, or you can set the camera to assume a Z-up coordinate system (see here).

wlinna commented 1 month ago

Okay, thanks for the clarification