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

viewPortHandler.setMaximumScaleX not working for horizontal bar chart #256

Closed
liuxuan30 opened this issue Jul 30, 2015 · 18 comments
Closed

Comments

@liuxuan30
Copy link
Member

I am trying to set max scaleX for horizontal bar chart via viewPortHandler.setMaximumScaleX, let's say the maxScaleX to be 3.

However when zooming in on xAxis for horizontal bart chart, it can zoom in as much I want.

After the investigation, I found in below function matrix.a is always 1, but matrix.d is keeping increasing as the correct scale in limitTransAndScale. It seems like we forgot to switch them for horizontal bart chart, or do you intend to do so?

    private func limitTransAndScale(inout #matrix: CGAffineTransform, content: CGRect?)
    {
        // min scale-x is 1, max is the max CGFloat
        // for horizontal bar chart, matrix.a is always 1
        _scaleX = min(max(_minScaleX, matrix.a), _maxScaleX)

        // min scale-y is 1
        _scaleY = max(_minScaleY, matrix.d)

Also, I want to propose could we change the gestures and their handlers to be 'internal' rather than 'private'?

The pinch gesture handler is private and I cannot override it to adapt for horizontal bar charts, though we could use is HorizontalBarChartView in BarLineChartViewBase and make some changes, just like performPanChange

I tried to fix it but I found it's more complicated than I thought. I added setMaximumScaleY, and have to change canZoomInMoreX and have to get chart type in ChartViewPortHandler in order to read maxScaleY for horizontal bar chart, which seems not a good design pattern.

Needs your help how to fix this.

@danielgindi
Copy link
Collaborator

About limitTransAndScale, the changes should probably be in a subclass of ViewPortHandler, flipping the functions. But I think we'll leave it be for now, as we may be changing the transformer's internal logic when switching to x-values instead of x-indexes

As for the gestures - I have no objection to making it internal...
We could go other way, checking with is, or making it in the subclass. If you see that there's too much overhead when doing it in a subclass, you could create a property for "rotated axis"

@liuxuan30
Copy link
Member Author

@danielgindi I was stuck in how to read correct _maxScaleX in ChartViewPortHandler. I have to obtain chart type in ChartViewPortHandler, which I think is not a good design? I want to read matrix.d instead of matrix.a for horizontal bar chart.

@danielgindi
Copy link
Collaborator

Yes it is, the better way should be that the ViewPort knows that the axis is rotated (x=y, y=x)

@liuxuan30
Copy link
Member Author

Got you. I will take a look at the subclassing way and 'rotated axis' way.

@liuxuan30
Copy link
Member Author

@danielgindi Another question, do you have any purpose for the missing _maxScaleY property? It seems you intend to omit it.

@danielgindi
Copy link
Collaborator

maxScaleY is just a much more complex issue :-)

@liuxuan30
Copy link
Member Author

I tried adding property axisRotated and just switch them like this in limitTransAndScale:

        if (_axisRotated)
        {
            // min scale-x is 1, max is the max CGFloat
            _scaleX = min(max(_minScaleX, matrix.d), _maxScaleX)

            // min scale-y is 1
            _scaleY = max(_minScaleY, matrix.a)
        }
        else
        {
            // min scale-x is 1, max is the max CGFloat
            _scaleX = min(max(_minScaleX, matrix.a), _maxScaleX)

            // min scale-y is 1
            _scaleY = max(_minScaleY, matrix.d)
        }

Other code in ViewPortHandler not changed.

It actually can zoom to correct maxScaleX, but while zooming, the chart is moving like 'shaking' a lot not static.

@danielgindi
Copy link
Collaborator

You might be encountering the problem of the axis changing its size. You
might want to specify its size customly.

On Thu, Jul 30, 2015 at 11:40 AM, Xuan [email protected] wrote:

I tried adding property axisRotated and just switch them like this in
limitTransAndScale:

    if (_axisRotated)
    {
        // min scale-x is 1, max is the max CGFloat
        _scaleX = min(max(_minScaleX, matrix.d), _maxScaleX)

        // min scale-y is 1
        _scaleY = max(_minScaleY, matrix.a)
    }
    else
    {
        // min scale-x is 1, max is the max CGFloat
        _scaleX = min(max(_minScaleX, matrix.a), _maxScaleX)

        // min scale-y is 1
        _scaleY = max(_minScaleY, matrix.d)
    }

Other code in ViewPortHandler not changed.

It actually can zoom to correct maxScaleX, but while zooming, the chart is
moving like 'shaking' a lot not static.


Reply to this email directly or view it on GitHub
#256 (comment)
.

@liuxuan30
Copy link
Member Author

I improved limitTransAndScale like below. The zooming is working to me, but when it exceeds the maxScaleX, let's say 3, the chart start moving to the top. I am not sure why, as far as I can tell is the matrix.ty is changing in pinchGestureRecognized:. Could you help?

I am not sure if below code about the newTransY calculation is correct.

This is the current limitTransAndScale:

    /// limits the maximum scale and X translation of the given matrix
    private func limitTransAndScale(inout #matrix: CGAffineTransform, content: CGRect?)
    {
        if (_axisRotated)
        {
            // min scale-x is 1, max is the max CGFloat
            _scaleX = min(max(_minScaleX, matrix.d), _maxScaleX)

            // min scale-y is 1
            _scaleY = max(_minScaleY, matrix.a)
        }
        else
        {
            // min scale-x is 1, max is the max CGFloat
            _scaleX = min(max(_minScaleX, matrix.a), _maxScaleX)

            // min scale-y is 1
            _scaleY = max(_minScaleY, matrix.d)
        }

        var width: CGFloat = 0.0
        var height: CGFloat = 0.0

        if (content != nil)
        {
            width = content!.width
            height = content!.height
        }

        if (axisRotated)
        {
            var maxTransX = -width * (_scaleY - 1.0)
            var newTransX = min(max(matrix.tx, maxTransX - _transOffsetX), _transOffsetX)

            var maxTransY = height * (_scaleX - 1.0)
            var newTransY = max(min(matrix.ty, maxTransY + _transOffsetY), -_transOffsetY)

            matrix.tx = newTransX
            matrix.a = _scaleY
            matrix.ty = newTransY
            matrix.d = _scaleX
        }
        else
        {
            var maxTransX = -width * (_scaleX - 1.0)
            var newTransX = min(max(matrix.tx, maxTransX - _transOffsetX), _transOffsetX)

            var maxTransY = height * (_scaleY - 1.0)
            var newTransY = max(min(matrix.ty, maxTransY + _transOffsetY), -_transOffsetY)

            matrix.tx = newTransX
            matrix.a = _scaleX
            matrix.ty = newTransY
            matrix.d = _scaleY
        }
    }

@liuxuan30
Copy link
Member Author

Well I spent a lot to figure out what's going on for it. Here's my investigation

Let's say we set maxScaleX to 3 for horizontal bar chart.

If I apply above code, the zoom in will stop when it reaches 3, however it will have some offset vertically. It is because, in pinchGestureRecognized, here's the code:

var scaleX = (_gestureScaleAxis == .Both || _gestureScaleAxis == .X) && _scaleXEnabled ? recognizer.scale : 1.0
var scaleY = (_gestureScaleAxis == .Both || _gestureScaleAxis == .Y) && _scaleYEnabled ? recognizer.scale : 1.0
var matrix = CGAffineTransformMakeTranslation(location.x, location.y)
matrix = CGAffineTransformScale(matrix, scaleX, scaleY)
matrix = CGAffineTransformTranslate(matrix, -location.x, -location.y)
matrix = CGAffineTransformConcat(_viewPortHandler.touchMatrix, matrix)
_viewPortHandler.refresh(newMatrix: matrix, chart: self, invalidate: true)

When calculating scaleY, once we reaches the max scale 3, the viewPortHandler.touchMatrix.d will be 3, however, the matrix.d will be some value larger than 1, let's say 1.2. Now, the matrix will concat the touchMatrix, so the new matrix.d after concat is 3.6(3*1.2), and matrix.ty is positive value. This matrix.ty is based on scale 3.6, not 3. so when we are trying to calculate it in limitTransAndScale,

var maxTransY = height * (_scaleX - 1.0)
var newTransY = max(min(matrix.ty, maxTransY + _transOffsetY), -_transOffsetY)

The newTransY will always be matrix.ty until it exceeds maxTransY. That's why I have the offeset vertically.

I think it is a tough bug to fix.

What I can think about is, we have to calculate the ty when the scale is exactly based on the max scale, and store it somewhere, this will be the correct value to use, after the matrix.d larger than mas scale.

@liuxuan30
Copy link
Member Author

@danielgindi I suddenly realize am I facing the same maxScaleY issue...? Since horizontal bar is rotated, and then the scaling the X for horizontal bar is just like sacaling Y for normal bars...

@danielgindi
Copy link
Collaborator

danielgindi commented Jul 31, 2015 via email

@liuxuan30
Copy link
Member Author

No, I mean I am stuck at the matrix calculating, and today I feel setting max scaling x scale feature for horizontal bar is just like setting normal charts' max scale on y axis.

Currently we don't support setting maxScaleY, and you said it's complicated, and it is the same complexicity for horizontal bar chart...

I now trying save the last touchMatrix before it exceeds the maxScale, and try to calculate the translations from this last touchMatrix to get the correct translateX,Y. But still some bugs.

Can you share where you are blocked for this feature? Do you have plan for fixing it :-)

@danielgindi
Copy link
Collaborator

@liuxuan30 does #388 solve this for you?

@liuxuan30
Copy link
Member Author

@danielgindi I need some clarifications here, for horizontal bar chart, what direction is for maxScaleX and maxScaleY?

I just tried play with them, but I found setting maxScaleX for horizontal bar chart's effect is like setting same maxScaleX as normal bar chart, which I think should be maxScaleY effect.

Which means, if I use below code in horizontal bar chart in ChartsDemo:

[_chartView.viewPortHandler setMaximumScaleX:3.0];

It actually works horizontally, which I think it should be y axis... This seems very different at the time we were talking about this post. I was always thinking the x axis for horizontal bar chart is the y axis for normal bar chart...

And

[_chartView.viewPortHandler setMaximumScaleY:3.0];

actually could limit the horizontal bar chart's 'x' axis. So I think this issue works for me, only the x and y axis are confusing.

BTW, I found another small bug #463: if you reach the max scale level, scroll the chart to the middle, and try zoom in again, it actually starts a scrolling to top, which at first seems zooming. This is also a bug when I tried to implement maxScaleY. So I use another way to solve my problem, meaning saving a touchMatrix everytime and read data from it.

@danielgindi
Copy link
Collaborator

The bug at #463 is fixed, but X/Y are still flipped for horizontal bar chart, as the axises ARE actually flipped. You are using Y axis component for the axis that shows on the X axis, but it is still called Y axis.
I think that when we move to x values instead of x indexes - that will change too as the Viewport itself will be the actual big change.

@liuxuan30
Copy link
Member Author

It works like a charm with b1e9d0b while setting maxScaleX/Y. A old though bug solved finally! Now I have to recall all my changes for this.

My misunderstanding is I use setMaximumScaleX for horizontal bar chart, so I have reverted the matrix calculation for horizontal bar chart, and manipulated pinchGestureRecognized. It seems I use a very complicated way to solve the problem.

@liuxuan30
Copy link
Member Author

@danielgindi Yes, the big change is for the transformers and view ports. I tried another sort of scatter chart, and I put one normal bar chart y axis and one horizontal bar chart y axis together to work as x and y axis in the scatter chart.

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

No branches or pull requests

2 participants