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

Feature Request: VictoryZoomContainer #143

Closed
agusvazquez opened this issue Oct 10, 2023 · 14 comments · Fixed by #413
Closed

Feature Request: VictoryZoomContainer #143

agusvazquez opened this issue Oct 10, 2023 · 14 comments · Fixed by #413
Assignees
Labels
enhancement New feature or request

Comments

@agusvazquez
Copy link

Description

Hello! I am migrating from victory-native v36 to this one and I was wondering if there are plans to implement the VictoryZoomController.

https://formidable.com/open-source/victory/docs/victory-zoom-container/

Also, is there any example of implementing the zoom behavior using chart gestures? I thought on detecting the taps and measuring the distance between them, but I'm not sure if that will be performant enough. Open to ideas.

https://formidable.com/open-source/victory-native/cartesian/chart-gestures

Thank you!

@aendel
Copy link

aendel commented Oct 11, 2023

I'm planning to do a migration as well as @agusvazquez , I would love to know about the VictoryZoomController or about any example of implementation with chart-gestures

@jigneshoxit
Copy link

any update on this?

@Facundo1609
Copy link

Hey! Will we have this feature in the near future?

@hoanguyen2011
Copy link

Hey! Any update for this feature? My team really enjoys working with victory-native. But we miss the pinch-zoom and pan gesture 😢

@MrAMunro
Copy link

+1 for this. It would be good to know if this is planned future work or if there is now some limitation preventing it from being possible.

@keithluchtel
Copy link
Member

No limitations on it being possible. It's just a complex feature that takes some time & thought. With that, I've begun playing around with some implementation ideas but do not have a timeline on when it'd be available.

@MrAMunro
Copy link

Thanks for the reply Keith. Is it possible to share your thoughts and ideas here? I am considering options if I'd like this developed sooner.

For now I've got back to using an iFrame wrapping a chartJS, as I find the deprecated victory library which contains the zoomcontainer doesn't seem to work on iOS or android anymore for pan or zoom.

@bcheidemann
Copy link

No limitations on it being possible. It's just a complex feature that takes some time & thought. With that, I've begun playing around with some implementation ideas but do not have a timeline on when it'd be available.

Hey @keithluchtel! I've also been playing around with this. I have capacity to work on it at the moment, so if you have any thoughts on the implementation, API, etc, I'd be very interested to hear them! 🙂

@keithluchtel
Copy link
Member

No limitations on it being possible. It's just a complex feature that takes some time & thought. With that, I've begun playing around with some implementation ideas but do not have a timeline on when it'd be available.

Hey @keithluchtel! I've also been playing around with this. I have capacity to work on it at the moment, so if you have any thoughts on the implementation, API, etc, I'd be very interested to hear them! 🙂

Hey @bcheidemann, thank you very much for putting this together! This is quite different from how I was going about the implementation, but I think yours is a lot simpler.

I was working on adding the gesture detection within the CartesianChart itself for panning and zooming, similar to what we do for chart presses already. From there, I was keeping track of a transform matrix and applying the transform to the chart to get the pan/zoom effects. I hadn't yet accounted for the axis updating.

What I like about yours is that you're accomplishing the pan/zoom just by updating the domains and having the chart recalculate the paths accordingly. With the current implementation, users would each have the manually add the gesture detection however.

I need to think and get some more opinions on ultimately how we want the API to change to accommodate the feature.

cc @zibs @carbonrobot

@bcheidemann
Copy link

Hey @keithluchtel, thanks for taking a look!

Hey @bcheidemann, thank you very much for putting this together! This is quite different from how I was going about the implementation, but I think yours is a lot simpler.

I was working on adding the gesture detection within the CartesianChart itself for panning and zooming, similar to what we do for chart presses already. From there, I was keeping track of a transform matrix and applying the transform to the chart to get the pan/zoom effects. I hadn't yet accounted for the axis updating.

Ultimately, I think it makes sense to use the existing GestureDetector to handle pan and zoom, as you suggest. My intention behind the PR was mainly to promote discourse on different approaches to the problem, test/demonstrate an approach (i.e. updating the domains to apply a pan and zoom effect), and to feel out the rough edges of that approach (there are a few!).

Keeping track of a transform matrix does make a lot of sense, and I had considered something similar myself. However, just thinking out loud here, I was wondering if it makes sense to instead track a "view box" for the chart in its native axes coordinates.

The advantages of this approach would be as follows:

  1. Calculating the adjustments to the view box from gesture events is still relatively simple
  2. We could use this to adjust domains, in which case the chart would recalculate paths and adjust axes automatically (I discuss why this might not be a good idea below)
  3. If more data is added to the chart while it is panned/zoomed, it won't result in the view box moving unexpectedly
  4. The view box can be surfaced as a prop and/or via a setViewbox method on the ref, to allow users to take control over pan/zoom

What I like about yours is that you're accomplishing the pan/zoom just by updating the domains and having the chart recalculate the paths accordingly. With the current implementation, users would each have the manually add the gesture detection however.

I do think letting the chart recalculate paths based on the domain has some advantages (mainly simplicity). However, there's also some limitations I found when playing around with this method. In particular, it links the animations with pan/zoom in a way that leads to an... unconventional pan/zoom experience! (see the video below)

Simulator.Screen.Recording.-.iPhone.15.Plus.-.2024-10-18.at.09.42.21.mp4

One solution to this is using context to disable all animations during pan/zoom. This probably works fine for most cases but breaks down in the following scenarios:

  1. Animations being interrupted by starting to pan/zoom
  2. Continuously animated charts would not animate during pan/zoom

Ideally, the pan/zoom transformations would be applied separately from the path (and other) animations. Although I have a couple of ideas for how this could be done, I'm not currently sure what the best approach would be. Certainly would interested if you have any thoughts on this!

I need to think and get some more opinions on ultimately how we want the API to change to accommodate the feature.

cc @zibs @carbonrobot

The more opinions the better! 🙂 I'm looking forward to hearing yours and others thoughts. To summarise my current opinions:

  1. I agree the CartesianChart component should handle pan/zoom touch inputs internally
  2. I feel that animations and pan/zoom transformations should be decoupled, allowing for paths (and other elements) to be animated while panning/zooming - this (I think) rules out the domain adjustment approach from feat: add pan and scale props for CartesianChart component #402.
  3. I think it could be useful to track pan/zoom in terms of the charts native co-ordinates (though I'm certainly not married to this idea)
  4. It would be nice if the pan/zoom functionality could be surfaced via props and/or ref, to offer more control where needed (but this is probably not needed for the initial implementation)

Hopefully the above is useful... as I'm sure you can tell, this is my first time working with the library, as well as delving into the source code. Please excuse me if anything above is dumb and/or unhelpful 😅

@keithluchtel
Copy link
Member

@bcheidemann I think we're converging on a similar idea on how this should work.

  1. I agree the CartesianChart component should handle pan/zoom touch inputs internally
  2. I feel that animations and pan/zoom transformations should be decoupled, allowing for paths (and other elements) to be animated while panning/zooming - this (I think) rules out the domain adjustment approach from feat: add pan and scale props for CartesianChart component #402.
  3. I think it could be useful to track pan/zoom in terms of the charts native co-ordinates (though I'm certainly not married to this idea)
  4. It would be nice if the pan/zoom functionality could be surfaced via props and/or ref, to offer more control where needed (but this is probably not needed for the initial implementation)
  1. Agreed, I think this will be simpler overall and I'm really close to having this piece done. Just having some issues with calculating the zoom focal point when doing multiple pan/zoom operations.
  2. I think with using a transformation matrix that manipulates the currently rendered canvas, we'll achieve this (maybe?)
  3. My current implementation is exposing pan/zoom via a hook similar to useChartPressState, with my new one being: useChartTransformState. From this hook we could expose some utility functions to allow manipulating the pan/zoom programatically.

My code is a mess right now, but I'll work on cleaning things up when I get a little time and push up to a feature branch to share my current work.

@bcheidemann
Copy link

@bcheidemann I think we're converging on a similar idea on how this should work.

  1. I agree the CartesianChart component should handle pan/zoom touch inputs internally
  2. I feel that animations and pan/zoom transformations should be decoupled, allowing for paths (and other elements) to be animated while panning/zooming - this (I think) rules out the domain adjustment approach from feat: add pan and scale props for CartesianChart component #402.
  3. I think it could be useful to track pan/zoom in terms of the charts native co-ordinates (though I'm certainly not married to this idea)
  4. It would be nice if the pan/zoom functionality could be surfaced via props and/or ref, to offer more control where needed (but this is probably not needed for the initial implementation)
  1. Agreed, I think this will be simpler overall and I'm really close to having this piece done. Just having some issues with calculating the zoom focal point when doing multiple pan/zoom operations.
  2. I think with using a transformation matrix that manipulates the currently rendered canvas, we'll achieve this (maybe?)
  3. My current implementation is exposing pan/zoom via a hook similar to useChartPressState, with my new one being: useChartTransformState. From this hook we could expose some utility functions to allow manipulating the pan/zoom programatically.

My code is a mess right now, but I'll work on cleaning things up when I get a little time and push up to a feature branch to share my current work.

Nice! This all sounds great 🙂 I look forward to checking out your branch - I'm sure I can learn a lot from seeing how you've implemented things!

It sounds like you've more or less got things under control but let me know if there's anything I can do to help.

@keithluchtel keithluchtel self-assigned this Oct 29, 2024
@keithluchtel keithluchtel mentioned this issue Oct 29, 2024
13 tasks
@keithluchtel
Copy link
Member

@bcheidemann following-up. I just pushed my current state to draft PR: #413 if you want to check it out and/or have any feedback.

@keithluchtel
Copy link
Member

Available in version 41.11.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants