-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add glTF draco support #2591
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
base: master
Are you sure you want to change the base?
Add glTF draco support #2591
Conversation
|
Thanks, from a first look it seems solid.
we have the formatter configs here (should work with eclipse too). But don't worry about that. Is it ok if i push some changes to your branch for the gradle dependency and to fix the formatting? |
Yes. About the GL constants: I can change/inline them. But think that some (Spoiler: That's the wrong value there 🙂 there's a reason why I once created https://javagl.github.io/GLConstantsTranslator/GLConstantsTranslator.html , but I think it's better to not have to use it) Do you think that it could make sense to define the required constants as |
Yes, that or on a dedicated "constants" class inside the loader package would be a good solution. |
|
I made some changes:
the ide should pick that up automatically, if not
|
| /** | ||
| * The default log level | ||
| */ | ||
| private static final Level level = Level.INFO; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
REMINDER: probably should set this to FINE or lower
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - it is pulled out into a constant (as opposed to using logger.fine... everywhere) exactly to have it be INFO during test and development, and be able to quickly switch it to FINE later.
|
I don't know if this is related with the other issues, but the sample "BarramundiFish" seems to have half of the mesh (or texture?) offsetted compared to the non draco compressed version |
| * The attribute data | ||
| * @return The vertex buffer | ||
| */ | ||
| private static VertexBuffer createFloatAttributeVertexBuffer(JsonObject accessor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this handle normalization too? Maybe it can repurpose GltfLoader.readAccessorData made public static and moved into GltfUtils?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Draco spec itself does not explicitly mention normalization. And I think that there is no asset where the a normalized accessor was draco-encoded. But I can try to create some test data and see what happens.
A bit more generally: There might be snippets of functionality that could have overlap with GltfLoader or GltfUtils, or where minor changes on either side allow functions to be used on the other side. I try to avoid redundancies, but don't have a perfect overview over all existing functions yet.
|
Hah! 💡 The broken At https://github.com/openize-com/openize-drako-java/blob/4d99a762fbd2a167922bf3e27535b5651e114a30/src/utils/dev/fileformat/drako/Unsafe.java#L159 it is only decoding the first element of the float array (apparently, the quantization offsets here). When changing this line to use That's good. It's a trivial fix. As mentioned in that issue: I could open a PR for that. But maybe there will be further feedback in that issue, so that we can sort out whether the other issues can also be fixed quickly. (In doubt, maybe we can push for a (BTW: I'll probably do another pass and further work on all this PR in the next days, e.g. during the weekend) |
|
@riccardobl I'm looking at that fork with a bit of scrutiny. It is changed completely (78 changed files). This fork can never be synced back to the original repo. And the actual change, literally changing an |
|
Nice! So i've forked it jMonkeyEngine/openize-drako-java, cleaned it up a bit, applied the fish-patch and redeployed it to github registry, this seems to have fixed also the issue with the "Lantern" sample. I've updated this PR to use the forked artifact on maven·rblb it (just a github registry proxy without authentication), we are likely going to deploy it to maven central later. If you want to test with a locally built artifact you can just and then replace these 3 lines with just |
|
^ There was a race condition in the comments. Again: I assume that this is a workaround for now, and maybe we can find a less disruptive solution here. |
🤣
the original code is transpiled and it was going to pull too much dependencies on nio, javax and other stuff at compile time and likely breaking on transpilation to javascript etc... most of the changes are just unused classes, dead code and random/wrong interface usage being removed, so i expect merging upstream changes shouldn't be too hard. But i wouldn't expect a transpiled library to be maintained tbh. |
|
Yeah. I have seen some of the code, and ... it raised a few question marks for me. But let's skip the details for now, and say that it's good that there is a pure-Java solution for decoding Draco. How exactly that code was created is also not clear. There apparently is an auto-generated part, but the However, we could speculate and argue back and forth, but let's see whether there is any feedback in the issue, and what could be a sensible path forward based on that. |

Addesses #2118
This is a first draft of adding support for the
KHR_draco_mesh_compressionextension when loading glTF.Summary
This adds a
DracoMeshCompressionExtensionLoaderclass. It is registered as the extension loader for theKHR_draco_mesh_compressionextension. When the main glTF loader is loading the mesh primitives inGltfLoader#readMeshPrimitives, it will call theDracoMeshCompressionExtensionLoaderto decode the Draco data that was found via the extension object.The Draco data is found in the
bufferViewthat is defined in the extension object. The loader will fetch the data from this buffer view, and pass it to theDraco.decodefunction of https://github.com/openize-com/openize-drako-java . The result will be aDracoMeshthat contains the decoded attribute data. This attribute data is then extracted and translated into the structure that is required for using it as jME mesh attributes. This decoded data will then replace the previous (empty) attributes of the mesh.The
TestGltfLoadingclass contains a small snippet to easily load the glTF sample assets that have a Draco version available. (This requires the respective glTF Sample assets to be copied into thejme3-testdataproject - instructions are in inlined comments).Remaining tasks
A pretty fundamental one: Where and how should the dependency to the draco decoding library be declared?
EDIT 2026-01-30, 12:46: This was resolved in the meantime. Riccardo added the dependency in 77d611f . There are a few open questions about issues in the
openize-drako-javalibrary, and in which version they will be fixed, but that will just be a version number update now.(Omitting some obsolete parts of my question here)
There are a few
TODO_DRACOmarkers in the code. These are mostly about functions that could be moved to different classes if they are considered to be more widely useful.I found some hints about the code formatting preferences. Is there some magic
gradle nowFormatEverythingProperlycommand for this? (For now, I used the default Eclipse formatting)
Open issues
These have been described in more detail in a comment in the issue. Just to quickly summarize them here:
SunglassesKhronosmodel cannot be loaded. The rason for that is likely also a bug in the Draco decoder, and also described in Failure to decode some Draco data from glTF Sample Assets openize-com/openize-drako-java#5 in more detailVirtualCitymodel cannot be loaded. I assume that the Draco-compressed version of this model is invalid. Details in Draco-compressed version of VirtualCity might be invalid KhronosGroup/glTF-Sample-Assets#264translation, then the translation appears to be wrong.EDIT 2026-01-29, 19:25 The last point is somewhat obsolete, as of #2591 (comment) , and can be solved with a trivial fix in the
openize-drako-javalibrary. (Removing additional details that I had mentioned here about the state of the investigation)closes #2118