-
Notifications
You must be signed in to change notification settings - Fork 235
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
Add GL upload function to Java interface #959
Conversation
* @throws IllegalArgumentException Any array that is not | ||
* <code>null</code> has a length of 0 | ||
*/ | ||
public native int glUpload(int texture[], int target[], int glError[]); |
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.
Just curious. Why are these parameters arrays?
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.
That's mentioned in the first post. And it is the most important "design decision" here. I'll elaborate that in a comment below.
The upside-down image is hugely more likely to be an application issue than an issue with |
A detail:
I'm aware of the abundance of issues related to "textures being upside-down" (and crutches like
But that's all (probably) unrelated to About the inlined comment:
In the C version, these parameters are pointers, like
In Java, there are no pointers or addresses. And we have to return (up to) three There are different options for this: Using arrays (as it is done here)This is a (somewhat quirky, but IMHO reasonable) way of emulating "pointers". It is still somewhat convenient:
It also most closely resembles the original API. Defining a structure to carry these valuesOne could create a
And then call the function like this:
But defining, documenting, and accessing such a structure always involves some overhead of which I'm not sure whether it brings reasonable benefit here. In the JavaScript bindings, the
This does raise some questions about the error handling - the The Python bindings do not seem to offer the |
(A small note: The build failure at https://github.com/KhronosGroup/KTX-Software/actions/runs/11837211122/job/32983720702?pr=959#step:17:53 looks like it is unrelated to this PR, but in doubt, I'll re-start/investigate) |
It is indeed unrelated to glUpload. It uploads the image data in the same memory order it is found in the input file which in KTX defaults to the first byte being the top-left corner (equivalent to KTXorientation of "rd"). But OpenGL's commonly used orientation has the first byte being the bottom left corner. Applications need to check the image orientation of the KTX file and, if differs from the coordinate system, they are using, take steps to show the image with the correct orientation. That can be by y-flipping the image (ugh!), changing the texture coordinates or using a texture transform. Your observation suggests that Babylon is not properly handling KTX files. It is obviously taking one of the steps I suggested when handling PNG files. Most likely it is y-flipping the image before uploading to WebGL. It is not doing anything for KTX files. You can test this theory by creating the file with |
Thanks for the explanation about the arrays. Agree they seem the best way to return the values in Java. The JS binding is done the way it is due to EMBIND. Maybe there is a better way. If so, now would be a good time to fix it as the upcoming release contains a major rewrite of the wrapper. Is there a python wrapper for OpenGL? |
Why do you say that? The error is due to this warning
which is turned into an error because CI uses -Werror or whatever the MSVC equivalent is. |
It seems to be handling them properly when they are part of glTF and used as textures. The preview being upside down may just be a detail. (And the fact that it is even possible to simply drag-and-drop a KTX file into an online viewer and have a preview is already tremendously helpful - at least, until I get my Java application together that is the reason for all the stuff that I'm doing here...). Maybe the fact that it's shown upside down could be tracked somewhere in https://github.com/babylonjs , but it doesn't seem to be critical.
I cannot say much about the JS bindings, and would have to take a closer look at the actual
There seem to be bindings at https://pyopengl.sourceforge.net/ but I'm even less familiar with Python than with JavaScript.
I see, indeed, that's before that "Update test log" failure (and likely causes it). I removed the unused variable - let's see whether the build passes now. |
I guess merging #957 caused the conflict. The Travis-CI failure is because the Java wrapper test is failing on macOS for some reason. |
I'll open an issue but first please run |
# Conflicts: # interface/java_binding/src/test/java/org/khronos/ktx/test/KtxTexture2Test.java
The merge conflict is a bit surprising (as this branch should be based on the other one), but it is resolved now in any case.
Two questions about the build failure on MacOS:
(If you cannot answer them from the tip of your head, then don't waste your time - I can try to investigate it further). The relevant parts from the build output are
Usually, a VM crash causes a file with the name About the flipped texture: (That may be a bit off-topic here, but we can move this information wherever it belongs) I converted a PNG into KTX2 using different commands, and looked at the resulting files with the babylon viewer, and their
I had a short look at the documentation and the source code, searching for In any case, I'm not sure what is "right" or "wrong" here, and whether it's an issue of Babylon or of KTX-Software. The fact that Here is an archive that contains...
|
ktxTexture_GLUpload expects a context to be current. If there isn't one it will most likely crash. The behavior should be the same on all platforms but it is somewhat dependent on the OpenGL implementation. It does not check for an active context because that is not only a platform dependent operation and depends on what is being used to set up contexts.
If the first time doesn't I can't see any reason the second time would.
Update test log was an attempt to upload logs on interest in the event of a build failure. Unfortunately the site it was uploading to seems to no longer be available. Also it does not currently attempt to upload the vm log because I was unaware of it.
It's Babylon. The spec says the default orientation is top down (a.k.a "rd"). So your first and second cases are the same. Babylon should be flipping it, if it using WebGL's default coordinate system. Otherwise it will be upside down, exactly as you are seeing. There is no bug in the KTX software. In the third case |
That would explain it. To me, this means that this function cannot reaonably be tested as part of the unit tests. I will keep the test cases that pass in invalid parameters (because the point of these tests is to ensure that these cases are cauught by throwing a Java exception before even trying to call the native function). But the case where it actually does call |
I did a quick search, and there are some related threads/issues. But I think that this drag-and-drop preview for KTX is not really "specified" in any way. They are not making claims about the "texture coordinates of that ('virtual') rectangle" that the texture is rendered on. I hope it's OK to ping @bghgary here: We're talking about the drag-and-drop preview of KTX files in the sandbox. And it could be confusing that the preview appears to be displayed upside down, compared to the corresponding PNG file: Not a big deal, but maybe worth mentioning... (EDIT: Test data and further infos are contained in the archive at the bottom of a previous comment) |
That seems reasonable. On the native and JS side I test these functions using the glloadtests and vkloadtests apps which use SDL. Emscripten compiles these apps so I can use them in Javascript. Java versions would have to be written. Isn't it possible to create an OpenGL context in the unittests? One other thing I forgot to mention is that the Is it not clear to me at all why the test crashed only on macOS before. Perhaps you aren't correctly handling a failure to get the function pointers and they weren't found on macOS. |
The KTX spec is very clear that the default orientation has In my view this is a bug that should be fixed. It is indeed confusing that the preview for PNG and KTX differs. |
About the tests and GL context:
That might be possible (with the disclaimer that I don't know much about how many of these things actually work in the ~"headless" world of CI builds). I think that it would require to declare a dependency to a Java library that provides OpenGL bindings, to actually call the functions that create a GL context to begin with. This also might be possible (and of course, this dependency would only be required for the unit tests, and it would not be a dependency for the Java KTX bindings in general!). But there are too many unknowns for me to ~"just try this out" right now.
I have a rough idea about how some of these things work (related to some some stuff that I did in my OpenCL bindings). But it's not clear for me how that translates to KTX and OpenGL. You mentioned
There is the call to About the flipped texture:
Preferably yes, because it is confusing. The reason why I said that it is "not a big deal" is that this only applies to the drag-and-drop preview. When used as textures, everything seems to be fine, so it looks like they are already properly handling the metadata/orientation in that case. (I haven't looked into the implementation details, though). |
From a quick look, it does look like the preview creates a texture that is Y-inverted from what the glTF loader is doing. I will confirm and make a fix. |
It is documented. Maybe you were looking at the on-line docs which will be for the last release. The function was added after the last release. and even it it was, it would be difficult to "map" something like this to the Java world (with function pointers, and considering that the OpenGL bindings for Java are just a different library that probably already hides much of the low-level functionality here...) The caller never sees any function pointers but does have to provide a pointer to the function to use to query OpenGL pointers. I'm pretty sure failure to call
If an OpenGL library or the function pointers could not be found, the call to ktxTexture*_GLUpload would return either I don't want to burden the unit tests with needing an OpenGL implementation so I'm going to merge this as is. How do you obtain JOGL or whatever you are using in your app? Do you have to download an additional package? |
Yeah, there are two-and-a-half OpenGL bindings for Java:
In all cases, it should usually be sufficient to add the respective dependency to the Maven POM. But unfortunately, there sometimes still is the hassle of the native libraries. (This is strongly related to #624 where we talked about how this could be solved for KTX. The task to draft a solution for the native library handling for the KTX Maven release is still on my plate. I have some clear ideas here. I "only" need to allocate time for that...) For the OpenGL bindings, the current state is roughly:
The last two points remain to be verified. But if the last point is correct, then the answer to
is: No! - ideally, you just have to add it as a dependency to the Maven POM.xml, and it magically works 🤞 A while ago, created viewers for glTF, once based on JOGL, once based on LWJGL version 2 and (not published) based on LWJGL version 3. For my KTX experiments, I intend to do something similar: I started a |
This is fixed in Babylon.js Sandbox now. |
Thanks @bghgary ! |
(This builds upon #957, so that should be merged first)
This adds bindings for the
ktxTexture_GLUpload
function to the Java interface.There certainly are a few degrees of freedom about how exactly that should be offered. And there is a trade-off between "trying to closely resemble the existing API" and "trying to create a nice API for Java". The main point here being that the function receives three pointers (to
int
values, essentially). And pointers don't exist in Java.A common (although not pretty) way is to emulate these with single-element arrays. So the call currently looks like this:
This will fill the given arrays with the values that are returned from the native layer, accordingly.
Proper testing may be difficult. The basic call conditions are checked in a unit test. But beyond that, really using the function will require an OpenGL context to be current, so that 1. requires an OpenGL binding for the test, and 2. can hardly happen during the standard test runs.
I tried it out in a very basic experiment. This experiment currently used https://www.lwjgl.org/ , but I'll probably also try it with https://jogamp.org/ . Given that LWJGL is the library behind https://libgdx.com/ and https://jmonkeyengine.org/ , that's likely to be a primary goal.
The result of that experiment is shown here...
... and I frankly couldn't care less that it's upside down. It works 🙃