Skip to content

axes3d panning #18536

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

Merged
merged 4 commits into from
Oct 6, 2020
Merged

axes3d panning #18536

merged 4 commits into from
Oct 6, 2020

Conversation

emilyfy
Copy link
Contributor

@emilyfy emilyfy commented Sep 21, 2020

PR Summary

not sure why axes3d panning with middle scroll button is not implemented. I'm putting it here and it works as far as I tested.

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and pydocstyle<4 and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@QuLogic
Copy link
Member

QuLogic commented Sep 21, 2020

Can you write a What's new entry for this? You also have flake8 issues that would need to be fixed.

@emilyfy
Copy link
Contributor Author

emilyfy commented Sep 22, 2020

@QuLogic sorry about that, I'm not very familiar with the proper formatting. I fixed the mentioned flake8 issues, but what's a what's new entry?

@WeatherGod
Copy link
Member

WeatherGod commented Sep 22, 2020 via email

@tacaswell tacaswell added this to the v3.4.0 milestone Sep 23, 2020
@tacaswell
Copy link
Member

Thanks for this @emilyfy ! I suspect the reason it was not implemented is no one had implemented it yet ;).

I checked this out and verified that it works as expected.

Given that this whole block of code is untested I am not going to block merging on that, but if you are feeling motivated, following the pattern in https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/testing/widgets.py is the path to go.


I agree that this is a big enough feature added that we need to make sure it gets advertised in the whats new!

@emilyfy
Copy link
Contributor Author

emilyfy commented Sep 27, 2020

I added a short what's new entry.
Sorry again, but I'm not sure what exactly to do for testing the block of code. Is there perhaps a more related example I can refer to, where they test the rotation or zooming in the same _on_move function maybe?

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works pretty well for me. I'm not sure we really have any good way to test 3D interactivity.

def do_event(tool, etype, button=1, xdata=0, ydata=0, key=None, step=1):
"""
Trigger an event
def mock_event(ax, button=1, xdata=0, ydata=0, key=None, step=1):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Git diff looks more complicated than it is. This just extracts the event creation out of do_event().

@timhoffm
Copy link
Member

timhoffm commented Oct 3, 2020

@emilyfy Sorry, for being a bit confusing. Writing the test was actually a bit more difficult than thought, because the do_event() mechanism cannot be applied to mouse operations targeting a 3D Axes. I took the liberty of pushing a test to your branch.

ping @tacaswell @QuLogic for re-review/merge.

@timhoffm timhoffm merged commit 96670aa into matplotlib:master Oct 6, 2020
@timhoffm
Copy link
Member

timhoffm commented Oct 6, 2020

Thanks @emilyfy! Congratulations on your first contribution to Matplotlib. We'd be happy to see you back some time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants