GitHubLionel / wxMathPlot

An enhanced version of the wxMathPlot component for wxWidgets
11 stars 4 forks source link

Bug in mpFX x-range calculations causes out-of-range GetY calls #41

Open DRNadler opened 1 month ago

DRNadler commented 1 month ago

I'm trying to graph a time-series with valid indices ~[0-165000]. mpFX correctly sets up m_bound from my GetMinX()/GetMaxX() functions. But mpFX::DoPlot is indexing before the series (calls GetY with negative x) here: // Get first point iylast = w.y2p(DoGetY(w.p2x(m_plotBoundaries.startPx)), m_UseY2Axis); startPx is set wrong, because m_posX is set to a negative value. I think this happens because the x scaling factor is computed differently then this line in mpWindow::Fit(): m_posX = (rect.Xmin + rect.Xmax) / 2 - (m_plotWidth / 2 + m_margin.left) / m_scaleX; Perhaps m_scaleX is computed without the margin? This may do with the fix introduced here by JL. Thoughts? PS: Also indexing past the end of the series several times.

DRNadler commented 1 month ago

I had to write a wrapper class to avoid these bugs as follows:

typedef double TimeseriesValueFetchFunction(const LogInstance_T &logInstance, int idx);
double TEaltitudeAtIdx(const LogInstance_T &logInstance, int idx) { return logInstance.sensorDerivedValues[idx].TEaltitudeM; }
double TEvarioAtIdx(const LogInstance_T &logInstance, int idx) { return logInstance.sensorDerivedValues[idx].TEvario; }

class LogInstanceTimeSeriesPlot_FX: public mpFX
{
    const LogInstance_T &logInstance;
    VisualizationRange vr;
    Generic_3D_BoundingBox<double> limits; // X,Y for current vr
    TimeseriesValueFetchFunction* const V_atIdx;
    double lastSaneValue;
    /// Set bounding box 'limits' for above visualization range 'vr'
    void SetLimits() {
        limits.InitializeBoundingBox(vr.startIdx,GetY(vr.startIdx),0);
        for(int idx=vr.startIdx+1; idx<=vr.endIdx; idx++)
            limits.UpdateBoundingBoxToInclude(idx,GetY(idx),0);
    }
  public:
    LogInstanceTimeSeriesPlot_FX(
        const LogInstance_T &logInstance_,
        const VisualizationRange &vr_,
        TimeseriesValueFetchFunction &V_atIdx_,
        const wxColour& color,
        bool useY2Axis_ = false
    ) :
        logInstance(logInstance_),
        vr(vr_),
        V_atIdx(V_atIdx_),
        mpFX(logInstance_.logfileName,mpALIGN_RIGHT,useY2Axis_)
    {
        // MathPlot initialization
        wxPen FXpen(color, 2, wxPENSTYLE_SOLID);
        SetDrawOutsideMargins(false);
        SetContinuity(true);
        SetPen(FXpen);
        SetSymbol(mpsNone);
        // Set bounding box 'limits' for initial visualization range 'vr'
        SetLimits();
    }
    virtual double GetMinX() { return limits.Xmin; }
    virtual double GetMaxX() { return limits.Xmax; }
    virtual double GetMinY() { return limits.Ymin; }
    virtual double GetMaxY() { return limits.Ymax; }
    virtual double GetY(double x) {
        int idx = (int)x;
        // Avoid MathPlot bugs fetching out-of-range
        if     (idx<vr.startIdx) return GetY(vr.startIdx);
        else if(idx>vr.endIdx)   return GetY(vr.endIdx);
        double r = V_atIdx(logInstance,idx);
        // catch crazy values (would should have been filtered out upstream, except...)
        if(r>10000 || r<-10000) {
            // ooops
            r = lastSaneValue;
        } else {
            lastSaneValue = r;
        }
        return r;
    }
    void SetDefaultVisualizationRange() {
        vr.SetDefaultVisualizationRange(logInstance);
        SetLimits();
    };
    void SetVisualizationRange(const VisualizationRange &newVr) {
        vr = newVr;
        SetLimits();
    };
};
GitHubLionel commented 1 month ago

Well difficult to help you. Could you make a simple test code (like in demo examples) with which I can reproduce the problem.

DRNadler commented 1 month ago

I'll try to do so today, Thanks...

DRNadler commented 1 month ago

Here's a very minimal example, hope it helps!

Important Note: First step to sorting this out is fix documentation of all the mpWindow and maybe plot members where units are not specified, which directly and always leads to bugs like this. I fixed m_bound when it tripped me up, but everything below (ie m_scaleX) should specify units and whether boundary is included... This bug is caused by inconsistent interpretation of what for example m_scaleX really includes...

/// Range within timeseries to visualize for analysis, by observation index
struct TimeseriesRange {
    int startIdx=0, endIdx=0;
    constexpr TimeseriesRange() = default;
    constexpr TimeseriesRange(int s, int e) : startIdx(s), endIdx(e) {};
};

class BuggyTimeseriesPlot_FX: public mpFX
{
    const TimeseriesRange  vr = TimeseriesRange(0,150000);
    const double maxY = 150.0;
  public:
    BuggyTimeseriesPlot_FX() : mpFX("Buggy Timeseries",mpALIGN_RIGHT)
    {
        wxPen FXpen(*wxRED, 2, wxPENSTYLE_SOLID);
        SetPen(FXpen);
        SetDrawOutsideMargins(false);
        SetContinuity(true);
        SetSymbol(mpsNone);
    }
    virtual double GetMinX() { return vr.startIdx; }
    virtual double GetMaxX() { return vr.endIdx;   }
    virtual double GetMinY() { return 0; }
    virtual double GetMaxY() { return maxY; }
    virtual double GetY(double x) {
        int idx = (int)x;
        // Asserts below to easily trap out-of-range indexing
        assert(idx>=vr.startIdx);
        assert(idx<=vr.endIdx);
        return maxY * x / vr.endIdx; 
    }
};
GitHubLionel commented 1 month ago

Ok, I test your sample with a simple : BuggyTimeseriesPlot_FX *buggy = new BuggyTimeseriesPlot_FX(); m_plot->AddLayer(buggy, true); and with date format on x axis. I have a bug when I undo zoom about year 3000 ! lol This is gmtime() function who is crashing. I can just add a try catch for that.

But you have right, documentation is not clear about scale range ...

DRNadler commented 1 month ago

I did not use date format on axis (no axis labels). Did you observe the assert failures showing the bug I see?

GitHubLionel commented 1 month ago

Well, finally, I think that you use mpFX in a wrong way. This class is for function that they have no x axis limit. See for example MySIN in Sample.h of demo project. You can see that we not define GetMinX() or GetMaxX() because there is not limits (the limit is given by the scale of the x axis).

So in your sample, I think that :

DRNadler commented 1 month ago

I have no bottomAxis... How do I do set X range within a plot layer? This is necessary for time-series...

DRNadler commented 1 month ago

If you breakpoint GetMinX in example, you will see GetMinX() and GetMaxX() ARE called for example bug.

GitHubLionel commented 1 month ago

Yes GetMinX is called and it use the default range : [-1, 1] over X and same for Y axis. Probably, the behaviour is not what you expect. I suppose that you don't want to have plot outside your bound (ie [0,150000] in your example). For that, mpFX is not the good candidate. mpFXYVector is probably better.

DRNadler commented 1 month ago

As GetMinX() and GetMaxX() are virtual, my implementations are called, so it is not using the default values...