Skip to content
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

Prevent range changing during drag #386

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vasilenkoalexey
Copy link

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;
                }
            }
        }

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

                    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 auto 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 chnaged during drag!");
@vasilenkoalexey
Copy link
Author

Hello, anybody here? @epezent

@epezent
Copy link
Owner

epezent commented Sep 14, 2022

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.

@vasilenkoalexey
Copy link
Author

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.

@epezent
Copy link
Owner

epezent commented Sep 17, 2022

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?

@vasilenkoalexey
Copy link
Author

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.

@epezent
Copy link
Owner

epezent commented Sep 17, 2022

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
Copy link
Owner

epezent commented Sep 17, 2022

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.

@vasilenkoalexey
Copy link
Author

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
Copy link
Author

This is indeed something we need to fix!

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

Nahor added a commit to Nahor/implot that referenced this pull request Nov 24, 2023
…#386)

- Make scrolling not affect the zoom level when near the axis limit
  constraint
- Keep the zoom-out speed constant when one side of the axis is against
  the limit constraint while the mouse is on the other side
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.

2 participants