njam / citygml-to-3dtiles

Convert from CityGML to Cesium 3D Tiles.
Apache License 2.0
160 stars 41 forks source link

Computation of BoundingBox with cartographic coordinates #46

Closed felix-mu closed 5 months ago

felix-mu commented 6 months ago

Hi, first of all thank you for your great work! While using your library I found that the computation of the Tileset-Region or rather the bounding box is done in cartesian coordinates resulting in an unexpected result when converting it back to cartogrpahic coordinates.

grafik

The bounding box is the reason for not being able to pick features to view their attributes like stated here: https://github.com/CesiumGS/cesium/issues/10367#issuecomment-1126047465

During my investigation I found out that the determination of the min/max points of the points of the bounding boxes in https://github.com/njam/citygml-to-3dtiles/blob/11c45eed69f1422c815160e1f4dff6efe112e2d6/src/geometry/BoundingBox.mjs#L38 uses cartesian coordinates and therefore it might be the case that the bounding box does not correspond to the original bounding box in the projected SRS any more.

My current quick and dirty workaround is to subclass your bounding box class and override the fromPoints function to use cartographic coordinates and afterwards transform the bbox to cartesian coordinates:

import BoundingBox from "citygml-to-3dtiles/src/geometry/BoundingBox.mjs";
// import Cesium from "cesium";

class BoundingBoxCartographic extends BoundingBox {
    constructor(min, max) {
        super(min, max);
    }

    static fromPoints (points) {
        if (points.length < 1) {
          throw new Error('Invalid number of points: ' + points.length);
        }
        let min = points[0].clone();
        let max = min.clone();
        points.forEach(point => {
          min.longitude = Math.min(min.longitude, point.longitude);
          min.latitude = Math.min(min.latitude, point.latitude);
          min.height = Math.min(min.height, point.height);

          max.longitude = Math.max(max.longitude, point.longitude);
          max.latitude = Math.max(max.latitude, point.latitude);
          max.height = Math.max(max.height, point.height);
        })
        return new BoundingBox(min, max);
      }
}

export default BoundingBoxCartographic;
let boundingBox = BoundingBoxCartographic.fromBoundingBoxes(boundingBoxes);
        boundingBox = new BoundingBox(
          Cesium.Cartesian3.fromRadians(
            boundingBox.getMin().longitude,
            boundingBox.getMin().latitude,
            boundingBox.getMin().height
            ),
          Cesium.Cartesian3.fromRadians(
            boundingBox.getMax().longitude,
            boundingBox.getMax().latitude,
            boundingBox.getMax().height
          )
        );

This results in an correct bounding box and selectable features:

grafik

njam commented 6 months ago

@felix-mu thanks for the bug report!

I'll try to fix it, but I might need your help :o

Here's a pull request: https://github.com/njam/citygml-to-3dtiles/pull/47 Could you take a look? Do you think it solves the problem?

felix-mu commented 5 months ago

@njam thank you very much for taking action so quickly! I tested the new version with the sample data from above and it seems to be working right now. Also I took a look at your changes in the code and as far as I can judge it should be fine like you changed it. Thank you very much!

njam commented 5 months ago

@felix-mu thanks for the feedback! I've published a bugfix version 0.1.6 to NPM.