Skip to content

Conversation

@javagl
Copy link

@javagl javagl commented Jan 29, 2026

Addesses #2118

This is a first draft of adding support for the KHR_draco_mesh_compression extension when loading glTF.

Summary

This adds a DracoMeshCompressionExtensionLoader class. It is registered as the extension loader for the KHR_draco_mesh_compression extension. When the main glTF loader is loading the mesh primitives in GltfLoader#readMeshPrimitives, it will call the DracoMeshCompressionExtensionLoader to decode the Draco data that was found via the extension object.

The Draco data is found in the bufferView that is defined in the extension object. The loader will fetch the data from this buffer view, and pass it to the Draco.decode function of https://github.com/openize-com/openize-drako-java . The result will be a DracoMesh that 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 TestGltfLoading class 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 the jme3-testdata project - 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-java library, 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_DRACO markers 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 nowFormatEverythingProperly
command 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:

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-java library. (Removing additional details that I had mentioned here about the state of the investigation)

closes #2118

@riccardobl
Copy link
Member

riccardobl commented Jan 29, 2026

Thanks, from a first look it seems solid.
The only minor thing that i would want to change is the use of the GL renderer constants inside the loader, we'd generally prefer to avoid that.

I found some hints about the code formatting preferences. Is there some magic
gradle nowFormatEverythingProperly
command for this? (For now, I used the default Eclipse formatting)

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?

@javagl
Copy link
Author

javagl commented Jan 29, 2026

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
if (componentType == GL_UNSIGNED_SHORT)
is more readable than
if (componentType == 5122)

(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
private static final int GL_UNSIGNED_SHORT = 5123;
directly in the class that uses them?
(Assuming that the main point is to avoid the import of the otherwise unused GL interface...)

@riccardobl
Copy link
Member

riccardobl commented Jan 29, 2026

Do you think that it could make sense to define the required constants as
private static final int GL_UNSIGNED_SHORT = 5123;
directly in the class that uses them?

Yes, that or on a dedicated "constants" class inside the loader package would be a good solution.

@riccardobl
Copy link
Member

I made some changes:

  1. Added the gradle dependency

And what is the magic Gradle command to ~"update everything" afterwards?

the ide should pick that up automatically, if not
./gradlew build
or
./gradlew build --refresh-dependencies

  1. Refactored the example to load either from the KhronosGroup/glTF-Sample-Assets cloned in $HOME or directly from github
  2. Formatted
  3. Added FIXME notes on the draco samples that fails

/**
* The default log level
*/
private static final Level level = Level.INFO;
Copy link
Member

@riccardobl riccardobl Jan 29, 2026

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

Copy link
Author

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.

@riccardobl riccardobl self-assigned this Jan 29, 2026
@riccardobl
Copy link
Member

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,
Copy link
Member

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?

Copy link
Author

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.

@javagl
Copy link
Author

javagl commented Jan 29, 2026

half of the mesh (or texture?) offsetted

Huh!? I didn't notice that at first glance...

jME Draco 0003

(Top=Uncompressed, bottom=Draco)

What's surprising here: This is a single mesh - so I think that it can only be related to the texture coordinates. I'll compare the original texture coordinates to the decoded one.

I'm starting to have the feeling that there might be some general ~"offset" that is hidden somewhere in the draco data. This might just be the quantization offset, and would explain both the messed-up positions as well as messed-up texture coordinates. I'll check if some sort of offset can be obtained from the decoded DracoMesh.

@javagl
Copy link
Author

javagl commented Jan 29, 2026

Hah! 💡

The broken BarramundiFish and the wrong translations for the other models (e.g. CesiumMilkTruck) are both caused by what I mentioned at the bottom of openize-com/openize-drako-java#5 :

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 d < ret.length, the models can be loaded.

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 1.4.3 release that contains that trivial fix, and wait for a 1.4.4+ release to fix the vertex skinning decoding)

(BTW: I'll probably do another pass and further work on all this PR in the next days, e.g. during the weekend)

@javagl
Copy link
Author

javagl commented Jan 29, 2026

@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 i to a d, is buried in 872 additions and 3,551 deletions. Even though it is a ~"temporary workaround for now", we have to see how to do this in a way that is less disruptive. I'd give the openize-drako-java developers a few more days to chime in on the issue. Then I'd open a PR for that trivial fix (suggesting a 1.4.3 release). And if that doesn't work, we could think about a "clean" fork, with a single commit of that fix, and a version number like 1.4.3-jme-hotfix or so...

@riccardobl
Copy link
Member

Nice!
I looked into the openize-drako-java library, and it seems like a transpilation from some C# library, it has also a bunch of random unused classes, likely from some c# framework that the transpilled pulled inside the codebase, for some reason.

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

git clone https://github.com/jMonkeyEngine/openize-drako-java
cd openize-drako-java
./gradlew publishToMavenLocal

and then replace these 3 lines

https://github.com/javagl/jmonkeyengine/blob/618c9fa86c1197b80d6404699d33e8ae12fa9f83/jme3-plugins/build.gradle#L13-L15

with just

mavenLocal()

@javagl
Copy link
Author

javagl commented Jan 29, 2026

^ 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.
In any case, I'll try to keep things rolling in the next few days (already spent a bit too much time here in the past two days, but it was kinda cool).

@riccardobl
Copy link
Member

riccardobl commented Jan 29, 2026

^ 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. In any case, I'll try to keep things rolling in the next few days (already spent a bit too much time here in the past two days, but it was kinda cool).

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.

@javagl
Copy link
Author

javagl commented Jan 30, 2026

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 utils/ part (that contained the error) was probably not auto-generated. One important point is: Iff they have some sort of automated process, then it's not unlikely that they could re-run that process (maybe with bugfixes) at any point in time, leading to hundreds of merge conflicts with a state that was modified manually or with another toolchain.

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.

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.

implement the KHR_draco_mesh_compression extension

2 participants