implot icon indicating copy to clipboard operation
implot copied to clipboard

Prevent range changing during drag

Open vasilenkoalexey opened this issue 1 year ago • 9 comments

Fix small range changing during axis drag (from left to right). This occurs because SetMin uses Range.Max value for validating range before it is set by calling SetMax. It is easy to observe with for example the following code

        int drag_direction = 0;
        for (int i = 0; i < IMPLOT_NUM_X_AXES; i++) {
            ImPlotAxis& x_axis = plot.XAxis(i);
            if (x_held[i] && !x_axis.IsInputLocked()) {
                drag_direction |= (1 << 1);
                bool increasing = x_axis.IsInverted() ? IO.MouseDelta.x > 0 : IO.MouseDelta.x < 0;
                if (IO.MouseDelta.x != 0 && !x_axis.IsPanLocked(increasing)) {
                    const double plot_l = x_axis.PixelsToPlot(plot.PlotRect.Min.x - IO.MouseDelta.x);
                    const double plot_r = x_axis.PixelsToPlot(plot.PlotRect.Max.x - IO.MouseDelta.x);
                    const double range = x_axis.Range.Size();
                    x_axis.SetMin(x_axis.IsInverted() ? plot_r : plot_l);
                    x_axis.SetMax(x_axis.IsInverted() ? plot_l : plot_r);
                    IM_ASSERT_USER_ERROR(range == x_axis.Range.Size(), "Range changed during drag!");
                    if (axis_equal && x_axis.OrthoAxis != NULL)
                        x_axis.OrthoAxis->SetAspect(x_axis.GetAspect());
                    changed = true;
                }
            }
        }

vasilenkoalexey avatar Aug 06 '22 18:08 vasilenkoalexey

Hello, anybody here? @epezent

vasilenkoalexey avatar Sep 13 '22 21:09 vasilenkoalexey

Hey, thanks for the PR and sorry for the slow response. I was sitting on this one because I was worried it might have cause some unintended side effects. This may seem like a simple change, but it is not and needs to be confirmed not to break several related features (e.g. axis locking, constraining, inversion, etc.).

I just tested it and it turns out that it does break things, specifically it breaks the minimum zoom constraint feature, which can be observed in the demo under Axes/Axis Constraints. When you zoom in all the way, it should reach the minium zoom width constraint and prevent continued zoom. That still works but now it begins to pan with continued scrolling, which is not desired.

There may be other side effects as well that I haven't discovered yet.

I suspect SetRange is not enforcing all constraints correctly or the way that SetMin/Max are.

epezent avatar Sep 14 '22 02:09 epezent

When you zoom in all the way, it should reach the minium zoom width constraint and prevent continued zoom. That still works but now it begins to pan with continued scrolling, which is not desired.

You right, but... If we want prevent scrolling at minimum zoom, then best way to do it - dont process scroll input at minimum zoom. But now its not implemented

    // SCROLL INPUT -----------------------------------------------------------

    if (any_hov && IO.MouseWheel != 0 && ImHasFlag(IO.KeyMods, gp.InputMap.ZoomMod)) {

        float zoom_rate = gp.InputMap.ZoomRate;
        if (IO.MouseWheel > 0)
            zoom_rate = (-zoom_rate) / (1.0f + (2.0f * zoom_rate));
        ImVec2 rect_size = plot.PlotRect.GetSize();
        float tx = ImRemap(IO.MousePos.x, plot.PlotRect.Min.x, plot.PlotRect.Max.x, 0.0f, 1.0f);
        float ty = ImRemap(IO.MousePos.y, plot.PlotRect.Min.y, plot.PlotRect.Max.y, 0.0f, 1.0f);

        for (int i = 0; i < IMPLOT_NUM_X_AXES; i++) {
            ImPlotAxis& x_axis = plot.XAxis(i);
            const bool equal_zoom   = axis_equal && x_axis.OrthoAxis != NULL;
            const bool equal_locked = (equal_zoom != false) && x_axis.OrthoAxis->IsInputLocked();
            if (x_hov[i] && !x_axis.IsInputLocked() && !equal_locked) {
                float correction = (plot.Hovered && equal_zoom) ? 0.5f : 1.0f;
                const double plot_l = x_axis.PixelsToPlot(plot.PlotRect.Min.x - rect_size.x * tx * zoom_rate * correction);
                const double plot_r = x_axis.PixelsToPlot(plot.PlotRect.Max.x + rect_size.x * (1 - tx) * zoom_rate * correction);
                x_axis.SetMin(x_axis.IsInverted() ? plot_r : plot_l);
                x_axis.SetMax(x_axis.IsInverted() ? plot_l : plot_r);

really, new plot_l and plot_r values calculated (and should produce a scroll) even at minimum zoom, But here undesirable effect of SetMin, SetMax calls prevent undesirable scrolling at minimum zoom. And SetRange "fairly" set new plot_l and plot_r values, thereby producing scroll effect. So, I think using SetRange here is more correctly then SetMin, SetMax but this requires a slight redesign of scroll process logic.

vasilenkoalexey avatar Sep 14 '22 15:09 vasilenkoalexey

After looking into this more closely, I'm not convinced it's a real issue. The amount of variation in the range is extremely small, essentially epsilon, and I'm not so sure it's even avoidable. In fact, your fix doesn't actually correct the problem -- SetRange still produces error and triggers the assert you demonstrated in your first post.

I'm curious -- what was the actual problem that prompted you to investigate this?

epezent avatar Sep 17 '22 16:09 epezent

I'm curious -- what was the actual problem that prompted you to investigate this?

I just develop SDR software using your great library... Typical parameters of my x-axis: range constraint about [400000.0, 2000000000.0], zoom constraint [1000, 2400000.0] and range size is 2400000.0 With these values, the effect of changing the range during dragging becomes very noticeable and annoying (its looks like waterfall horizontal scaling during freq retuning by mouse drag). So... I would like fix this problem :)

In fact, your fix doesn't actually correct the problem -- SetRange still produces error and triggers the assert you demonstrated in your first post.

You right, my bad, I didn't fully test solution. But I think this is due to a rounding error during plot_l and plot_r calculation. At least I see an assert befroe SetRange calling at this example

IM_ASSERT_USER_ERROR(ImAbs(plot_l - plot_r) == x_axis.Range.Size(), "Range changed during drag!");
x_axis.SetRange(x_axis.IsInverted() ? plot_r : plot_l, x_axis.IsInverted() ? plot_l : plot_r);
IM_ASSERT_USER_ERROR(range == x_axis.Range.Size(), "Range changed during drag!");

So... I need some more time to think about a more complete solution. Or maybe you can suggest solution or some workaround for this.

vasilenkoalexey avatar Sep 17 '22 21:09 vasilenkoalexey

I just tried the following with the parameters you provided and don't see anything particularly unusual:

if (ImPlot::BeginPlot("My Plot")) {
    ImPlot::SetupAxisLimitsConstraints(ImAxis_X1, 400000.0, 2000000000.0);
    ImPlot::SetupAxisZoomConstraints(ImAxis_X1, 1000, 2400000.0);
    ImPlot::EndPlot();
}
  1. Could you provide a video and minimal example to reproduce the issue so I can better understand the behavior?
  2. If you scale your data down by a factor of say, 1000, does your experience improve? Ideally you don't have to, but this may provide a more pleasurable experience in the interim.

epezent avatar Sep 17 '22 21:09 epezent

Ah, nevermind I'm seeing the issue now. Yea, looks like the range does creep up pretty quickly with these values. This is indeed something we need to fix!

Thanks for the clarification.

epezent avatar Sep 17 '22 22:09 epezent

Great, thanks!

I remove changes from scrolling and selection because I'm not sure if it's needed. Solution like this

if(x_axis.IsInverted()) {
    const double plot_r = x_axis.PixelsToPlot(plot.PlotRect.Max.x - IO.MouseDelta.x);
    x_axis.SetRange(plot_r, plot_r - x_axis.Range.Size());
} else {
    const double plot_l = x_axis.PixelsToPlot(plot.PlotRect.Min.x - IO.MouseDelta.x);                        
    x_axis.SetRange(plot_l, plot_l + x_axis.Range.Size());
}

looks good for me. What you think?

vasilenkoalexey avatar Sep 18 '22 18:09 vasilenkoalexey

This is indeed something we need to fix!

Hello @epezent! Any news about this 1-year issue?

vasilenkoalexey avatar Oct 11 '23 20:10 vasilenkoalexey