A good start for ImGuizmos #41
Conversation
|
Seems like all builds were fine other than the check style for some of the native functions. I don't think it's that big of a concern at the moment. |
There was a problem hiding this comment.
Great stuff! Thanks for your contribution, I very appreciate your input.
Problems I see from the first look:
- Cleanups.
imgui-binding/tmpshould not exist. As well as shadow plugin etc. If you need to test out your code - use example module. It could be started with locally built libraries. - Speaking about example: you need to provide one. As it's done for node-graphs bindings. It should demonstrotate how to use binding in Java ecosystem. Number of features you can show there depends on you, but it's required to exist for sure.
- It's
static native void, notnative static void. Please, keep code consistency.
Generally speaking, code is fine, but you need to pay more attention to details. And to revamp your code so it will follow style of other parts of the binding. I see that it's a draft implementation, so please mark your PR title in an appropriate way. Or make the PR itself as a draft (github has such functionality). Other than that, good job!
| private native static void nEnabled(boolean[] buffer); /* | ||
| ImGuizmo::Enable(&buffer[0]); | ||
| */ | ||
|
|
||
| /** | ||
| * This will set the orthographic view | ||
| */ | ||
| public static void setEnabled(boolean isEnabled) { | ||
| buffer[0] = isEnabled; | ||
| nEnabled(buffer); | ||
| } |
There was a problem hiding this comment.
Very strange code. Just pass boolean, don't use array.
| setDrawList(ImGui.getWindowDrawList()); | ||
| } | ||
|
|
||
| private native static void nBeginFrame(); /* |
There was a problem hiding this comment.
You don't need this method. Just make public static native void beginFrame and call native function there. So it should be like call native method not call java method->which calls native method. Java layer is needed to do custom logic, like extraction of internal data from types ImFloat etc. Other than that, do direct native calls.
| * This will allow for manual editing of the guizmos | ||
| */ | ||
| public static void decomposeMatrixToComponents(float[] matrix, float[] translation, float[] rotation, float[] scale) { | ||
|
|
| ImVec2 pos = ImGui.getWindowPos(); | ||
| ImVec2 size = ImGui.getWindowSize(); |
There was a problem hiding this comment.
Both of those methods create new objects. In binding I avoid unnecessary allocations as much as possible. You should do something like that
imgui-java/imgui-binding/src/main/java/imgui/internal/ImGuiDockNode.java
Lines 651 to 660 in 0ed891f
| /** | ||
| * This is just the java representtation of the imguizmo operation | ||
| */ | ||
| public enum Operation { |
There was a problem hiding this comment.
Do not use enums. See any class in flag package to get the idea.
| /** | ||
| * This will allow for manual editing of the guizmos | ||
| */ | ||
| public static void recomposeMatrixFromComponents(float[] matrix, float[] translation, float[] rotation, float[] scale) { |
There was a problem hiding this comment.
Arguments order should be the same as for native method. Not something randomly generated. 😕
| * This will allow us to draw an arbitrary cube in the world. | ||
| */ | ||
| public static void drawCubes(float[] view, float[] projection, float[]... cubeMatrices) { | ||
| float[] matrices = new float[cubeMatrices.length * 16]; |
There was a problem hiding this comment.
Speaking about unnecessary allocations, that is the case too.
|
Hi @MrManiacc, Thanks for your contribution! |
Hi, feel free to take over. I'm going to be very busy for the next couple of weeks. |
|
Implemented in #54 |
I've implemented the base/required functions to get the ImGuizmos library running. I plan to add support for things like ImGradients etc.