SpaiR / imgui-java

JNI based binding for Dear ImGui
MIT License
581 stars 90 forks source link

ImPlot.setNextPlotTicksY() not working / not correct implemented #66

Closed RaphiMC closed 1 month ago

RaphiMC commented 3 years ago

How should this method be used? If I try to use it it always crashes java no matter what arguments I use. I wated to use the method like here: https://github.com/epezent/implot/blob/555ff688a8134bc0c602123149abe9c17d577475/implot_demo.cpp#L415 It takes two arrays and the size which there is no java wrapper apparently

SpaiR commented 3 years ago

Can you give the crashreport or any other usefull information about? Code you're trying to run etc. Without that every suggestion will be a guessing.

Personally, I don't use ImPlot and know nothing about how to use it. The API itself was provided by @perrymacmurray, so I assume he can give some advise about that. But information about the context of the problem is still required.

You can also try to investigate into the problem by yourself and if there is a problem with the binding API, to make a PR with a fix though.

perrymacmurray commented 3 years ago

There may be two problems here - I don't know why setNextPlotTicksY would be crashing, so please provide a crash report if you can.

It looks like only SetNextPlotTicksX(double, double, int, char*[], bool) is implemented of the two overrides. For your use case, depending on what exactly you're doing, this is fine: just specify the starting and ending points, and ImPlot will evenly space n_ticks ticks in that area. If you plan on using ticks that are not evenly spaced, or more than four labels (more below on that limitation), then you definitely need to use the other override.

It's been a little while, so I don't fully remember, but think I didn't implement the other override due to the difficulty of passing some arrays through JNI. I know that double[] can be passed with no problems, but passing a String[] does not properly convert the Strings to char*s. This is why setNextPlotTicksX can internally only accept four labels with the current implementation (the public method unpacks the array for JNI):

public static void setNextPlotTicksX(final double xMin, final double xMax, final int nTicks, final String[] labels, final boolean keepDefault) {
    final String[] labelStrings = new String[4];
    for (int i = 0; i < 4; i++) {
        if (labels != null && i < labels.length) {
            labelStrings[i] = labels[i];
        } else {
            labelStrings[i] = null;
        }
    }

    nSetNextPlotTicksX(xMin, xMax, nTicks, labelStrings[0], labelStrings[1], labelStrings[2], labelStrings[3], keepDefault);
}

private static native void nSetNextPlotTicksX(double xMin, double xMax, int nTicks, String s1, String s2, String s3, String s4, boolean keepDefault); /*
    char* strings[] = {s1, s2, s3, s4};
    ImPlot::SetNextPlotTicksX(xMin, xMax, nTicks, strings, keepDefault);
*/

If you're feeling up to it, you can definitely implement the other override of SetNextPlotTicksX and Y yourself, it should be almost identical code between the two. Otherwise, I'm feeling a bit more confident with JNI and can also give it a shot. I don't have a massive amount of time right now, though, so I can't guarantee when I'll finish with it.

RaphiMC commented 3 years ago

This is the crash reason:

#
# A fatal error has been detected by the Java Runtime Environment:
#
#  EXCEPTION_ACCESS_VIOLATION (0xc0000005) at pc=0x000000006e2316e2, pid=13768, tid=0x0000000000001a54
#
# JRE version: Java(TM) SE Runtime Environment (8.0_181-b13) (build 1.8.0_181-b13)
# Java VM: Dynamic Code Evolution 64-Bit Server VM (25.71-b01-dcevmlight-26 mixed mode windows-amd64 compressed oops)
# Problematic frame:
# V  [jvm.dll+0x1216e2]
#
# Failed to write core dump. Minidumps are not enabled by default on client versions of Windows
#
# An error report file with more information is saved as:
# C:\Users\Koppe\IdeaProjects\DeepClient\run\hs_err_pid13768.log
#
# If you would like to submit a bug report, please visit:
#   https://github.com/dcevm/dcevm/issues
#

This is the code which invokes the method:

ImPlot.setNextPlotLimits(0, 100, -0.5, playingSongValues.length - 0.5, ImGuiCond.Always);
ImPlot.setNextPlotTicksY(0, playingSongValues.length, 1); // This causes the crash
if (ImPlot.beginPlot("My Plot", "Level", "Instrument", new ImVec2(-1, 0), 0, 0, 0)) {
    Integer[] indexes = new Integer[playingSongValues.length];
    for (int i = 0; i < playingSongValues.length; i++) indexes[i] = i;
    Color c = ColorUtils.getRainbowColor(0, 5);
    ImPlot.pushStyleColor(ImPlotCol.Fill, ImColor.intToColor(c.getRed(), c.getGreen(), c.getBlue()));
    ImPlot.plotBarsH("Test", playingSongValues, indexes);
    ImPlot.popStyleColor();

    ImPlot.endPlot();
}

If I remove the call to setNextPlotTicksY the jvm doesn't crash anymore. Also I would need to override where I specify more 4 labels. I sadly can't add it because I don't know much about JNI.

perrymacmurray commented 3 years ago

Thanks for sharing - I'll see if I can figure out what's going on with the crash, and implement the override if I can

perrymacmurray commented 3 years ago

Sorry this has taken me so long. I finally have some time on my hands, and am starting to work through it all.