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

Change TRS+matrix floating-point precision to double #125

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tf2mandeokyi
Copy link

When reading Google Earth’s building models with de.javagl:jgltf-model:2.0.3, I encountered floating-point precision errors in the .glb models. Specifically, the TRS (Translation, Rotation, Scale) and matrix transformation values were read using float, providing only 7 digits of precision. Since model nodes are translated with the distance of the Earth's diameter (6,378,000m), this loss (range of error = -1m ~ 1m) becomes problematic in games (like Minecraft), where even minor discrepancies (0.2m) of model alignments can result in noticeable rendering artifacts.

Google Earth Web Minecraft
image image

This PR modifies only the TRS values, matrix values, and their relevant calculations (MathUtils, BoundingBox, ...) to use double instead of float. By increasing the precision from 7 to 15 digits (for example, the translation vector will change from [3258869.8, -1158451.4, -4877243.5] to [3258869.7866948475, -1158451.3178201506, -4877243.302543961]), large coordinate values are handled accurately, eliminating the floating-point errors observed during rendering.

@javagl
Copy link
Owner

javagl commented Jan 17, 2025

I appreciate the effort and thoroughness of this contribution. And I fully understand the underlying problem. In fact, I also stumbled over that, in the context of a (currently 'private') project called "J3DTiles". There, I already did use a double[] as the tile.transform matrix.

And I'm strongly in favor of changing float to double in JglTF as well. (I should have done this right from the beginning, but didn't. That was a mistake. I was young...).

But it is a severe, wide-ranging, breaking change. So I cannot just hit "merge" here. This will have to be part of a larger update - which will likely be a "JglTF 3.0.0".

And in this context, I'd probably apply this change even more thoroughly: You may have seen the note in the classes in the ...impl packages, saying

/*
 * glTF JSON model
 * 
 * Do not modify this class. It is automatically generated
 * with JsonModelGen (https://github.com/javagl/JsonModelGen)
 * Copyright (c) 2016-2021 Marco Hutter - http://www.javagl.de
 */

I already do have a (local) state of the https://github.com/javagl/JsonModelGen project that allows setting some
classGeneratorConfig.setNumberType(Double.class);
which will cause it to use double as the default type for JSON numbers throughout the generated classes.

Otherwise, the classes would use some mix of float and double, and each appearance of float might cause the same problem and have to be updated to double later - again resulting in a breaking change. I think that one (breaking) change to consistently use double would be preferable here.

@tf2mandeokyi
Copy link
Author

Thanks for the detailed and thoughtful response. I understand the concerns about introducing breaking changes—it makes sense to handle this as part of a larger, consistent update like a potential "JglTF 3.0.0".

That said, would it be feasible to create a separate branch with such setNumberType(Double.class) changes and potentially publish it as a dedicated Maven artifact (e.g., jgltf-model:2.0.4-dfp or jgltf-impl-v2:2.0.4-dfp)? This could serve as a temporary solution for projects (like mine) that require such change sooner while still aligning with the longer-term plan for a comprehensive update.

I understand if that’s not practical, but I thought it might be worth exploring. Thanks again for considering this and for the amazing work on the library :)

@javagl
Copy link
Owner

javagl commented Jan 17, 2025

That said, would it be feasible to create a separate branch [...] and potentially publish it as a dedicated Maven artifact

This might be possible in theory, but I'll have to investigate a few things here. Given that the impl-classes are "the basis of everything", I assume that this will affect nearly every other class (probably far more than the 32 files that are currently affected by this PR). Many of the changes will be "trivial" - just replace float/Float with double/Double everywhere, and remove some f's here and there. But I'd like to get an idea about how large that change really is.

Even if this was done, I'd wonder about a few aspects of the versioning and release. It should be a "preview-release" that is clearly marked as such. And I think that appending an arbitrary string to the version number, like 2.0.4-dfp, would not solve it. Depending on some details of Maven and the expectations of users, this might be perceived as a "compatible" (non-breaking) update of the 'patch' version. I'd rather lean towards some 3.0.0-SNAPSHOT or so to make the breaking nature of the change more obvious (with details to be sorted out - it would not be in Maven Central, but some sonatype "snapshots" repo).

Also, I have some local branch with progress for #109 (comment) that will also heavily be affected by this, and I'll think about how to sync that. It may be that the "Extension Support" also involves breaking changes, and it would be nice to solve this "in one pass". But I fully understand the desire to have a "preview" version with double as quickly as possible, even if it may require some additional alignment for the possible final 3.0.0 release.

I'll try to allocate some time to take a closer look at all this.

@javagl
Copy link
Owner

javagl commented Jan 18, 2025

@tf2mandeokyi I created a DRAFT PR which applies this change throughout the API, at #126

I can not yet say whether or how this could become of a "release" in any form. (All this is a one-man, spare-time project). But if you want to try it out, it should be possible to just run mvn clean package or mvn clean install to use this state for local, preliminary tests.

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

Successfully merging this pull request may close these issues.

2 participants