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

The upgrade does not properly handle gltfUpAxis #165

Open
javagl opened this issue Dec 27, 2024 · 1 comment
Open

The upgrade does not properly handle gltfUpAxis #165

javagl opened this issue Dec 27, 2024 · 1 comment

Comments

@javagl
Copy link
Contributor

javagl commented Dec 27, 2024

One legacy feature of 3D Tiles (which, I think, even predates 3D Tiles 1.0), was the possibility to specify an "up-axis" for the glTF assets that it referred to (indirectly, e.g. via B3DM). This was originally caused by the lack of an "up-axis" convention in glTF itself.

Now, there may still be legacy tilesets that specify the gltfUpAxis like this:

  "asset": {
    "gltfUpAxis": "Z",
    "version": "1.0"
  },

Right now, the upgrade command does not handle this property at all.

The upgrade command should check for this property, and

  • remove it from the asset
  • take it into account when converting the B3DM to GLB

The latter is a bit tricky. It is technically simple, or even trivial: Just throw in a few axis conversion matrices here and there. But it is highly error-prone, due to certain corner cases. For example: It might be that a glTF 1.0 asset uses the CESIUM_RTC extension, or the batch table defines an RTC_CENTER (or... maybe even both 🤪 ). These RTC centers are already taken into account during the upgrade, and they already anticipate the Y-up-to-Z-up conversion of glTF vs. 3D Tiles. So one has to be very, very careful to not mix up some axes when the gltfUpAxis has to be taken into account in addition to that. The exact order of transformations will have to be derived from the (legacy) specifications (or maybe even from the CesiumJS implementation), and verified with dedicated spec data sets.

A very quick draft of handling the case that the input contains gltfUpAxis=Z is in the branch up-axis-handling-draft, with all relevant changes in the commit b5dab25 . But this makes the assumption(!) that the input has gltfUpAxis=Z, and is not a general solution.

@javagl
Copy link
Contributor Author

javagl commented Jan 4, 2025

I started to create test data for this.

The case of a glTF 1.0 asset that uses the CESIUM_RTC extension will probably not be included. It's hard to cover this extensively. While creating glTF 1.0 is doable with https://github.com/javagl/JglTF , adding the extension will affect the Shader object (i.e. the Techniques and the GLSL shader string), so this would require some manual tweaks. On the one hand, adding this case could be justified, because it does add another dimension to the space of possible errors. On the other hand, it's exactly that "combinatorial explosion" that I'd like to avoid here. I don't know whether things like CESIUM_RTC extension plus RTC_CENTER in the batch table ever existed, and if they did, I doubt that one could reliably derive an unambiguous meaning of that from the specification...

I'll attach the current state of the test data (together with the Sandcastle) here:

Axes-2025-01-04a.zip

It tries to cover some combinations from

[glTF 1 with y/z up, 2] × [gltfUpAxis none, y, z] × [RTC none/(1,2,3)]

Cesium Axes 2025-04-01

Not all of them will be covered. When there ever is a data set with a glTF 1.0 with x-up (?) that declares gltfUpAxis=y, then I'd not consider this as "legacy data", but just as "wrong data".

The data will probably end up as a set of tests, using the "golden" approach that is already used for the 'migration' specs.

('m glad that I once created that Axes.glb - a small, reliable, unit-cube-shaped (!) glTF where it's possible to see the position, scaling, and orientation...)

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

No branches or pull requests

1 participant