-
Notifications
You must be signed in to change notification settings - Fork 65
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
base: master
Are you sure you want to change the base?
Conversation
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 And I'm strongly in favor of changing 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
I already do have a (local) state of the https://github.com/javagl/JsonModelGen project that allows setting some Otherwise, the classes would use some mix of |
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 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 :) |
This might be possible in theory, but I'll have to investigate a few things here. Given that the 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 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 I'll try to allocate some time to take a closer look at all this. |
@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 |
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 usingfloat
, 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.This PR modifies only the TRS values, matrix values, and their relevant calculations (
MathUtils
,BoundingBox
, ...) to usedouble
instead offloat
. 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.