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

[ImagePane] Addition of directional zoom + pan via mouse drag #421

Closed
wants to merge 5 commits into from

Conversation

clement-masson
Copy link
Contributor

Description

Added 2 features to the ImagePane Component.
The first one allows to zoom on a particular area of the image (while keeping this area in the center of the visible part)
The second allows to pan the image with the mouse drag action.
Most of the changes are in ImagePane.js. Only two lines of css needed to be changed to allow zooming to work properly.

Motivation and Context

Image interaction was quite poor so far : zoom and pan were not very user-friendly.
#308 introduced image pan, but it used onWheel event, which did not allow for horizontal pan on desktop computers.
In addition, it was not possible to zoom in a particular area of the image.

How Has This Been Tested?

I tested my changes by compiling the js code, running a visdom server, and displaying a saved visdom environment which included a visdom.image call.

I am using a line of javascript which might not be very clean :
this._paneRef._windowRef.children[1] at line 71 of file ImagePane.js, to get the top and left position of the Pane. There may be a cleaner way to get these but I couldn't find. As it uses some "hidden" attributes, it may not be very robust to future changes of React API.

My changes should not modify existing features, apart from the zooming and panning speed that I increased in the handleZoom function (feel free to reset these constants to their previous value).

I tried to run python example/demo.py but it failed with ConnectionRefusedError: [Errno 111] Connection refused, both with and without my changes (after installing from sources).

Screenshots (if appropriate):

Not appropriate

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • For JavaScript changes, I have re-generated the minified JavaScript code.

Copy link
Contributor

@JackUrb JackUrb left a comment

Choose a reason for hiding this comment

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

This seems like a solid improvement to me. Thanks for the contibution! Can you rebuild the js once #418 is in? I'll pull it in afterwards.

Also for testing with the demo, be sure to be running the server when launching the demo script.

@clement-masson
Copy link
Contributor Author

Ok I'll do that.

Also for testing with the demo, be sure to be running the server when launching the demo script
Lol how dumb I am. I somehow must have thought it was done automatically but did not even thought of it.
It indeeds works like a charm.

malmaud and others added 3 commits July 17, 2018 12:34
Summary:
Closes fossasia#415.
Pull Request resolved: fossasia#418

Differential Revision: D8875176

Pulled By: JackUrb

fbshipit-source-id: 38ab0fde5bfca867f7415090e4331e8be0af481d
Summary:
I was getting this error before:

```python
> visdom_client.contour(torch.randn(5,5))
```

```
~/anaconda3/lib/python3.6/json/__init__.py in dumps(obj, skipkeys, ensure_ascii, check_circular, allow_nan, cls, indent, separators, default, sort_keys, **kw)
    229         cls is None and indent is None and separators is None and
    230         default is None and not sort_keys and not kw):
--> 231         return _default_encoder.encode(obj)
    232     if cls is None:
    233         cls = JSONEncoder

~/anaconda3/lib/python3.6/json/encoder.py in encode(self, o)
    197         # exceptions aren't as detailed.  The list call should be roughly
    198         # equivalent to the PySequence_Fast that ''.join() would do.
--> 199         chunks = self.iterencode(o, _one_shot=True)
    200         if not isinstance(chunks, (list, tuple)):
    201             chunks = list(chunks)

~/anaconda3/lib/python3.6/json/encoder.py in iterencode(self, o, _one_shot)
    255                 self.key_separator, self.item_separator, self.sort_keys,
    256                 self.skipkeys, _one_shot)
--> 257         return _iterencode(o, 0)
    258
    259 def _make_iterencode(markers, _default, _encoder, _indent, _floatstr,

~/anaconda3/lib/python3.6/json/encoder.py in default(self, o)
    178         """
    179         raise TypeError("Object of type '%s' is not JSON serializable" %
--> 180                         o.__class__.__name__)
    181
    182     def encode(self, o):

TypeError: Object of type 'float32' is not JSON serializable
```

The problem is in  https://github.com/facebookresearch/visdom/blob/291a02e885a924c3e3cacc46fb059e9624ff5644/py/visdom/__init__.py#L1358 .

`X.min()` can return a numpy scalar type, like `numpy.float32`, which `json.dumps` doesn't know how to handle with the default serializer.
Pull Request resolved: fossasia#423

Differential Revision: D8879895

Pulled By: JackUrb

fbshipit-source-id: 2eaf4ab74dccf6c8fb33cdc399061ed70d61d72a
Conflicts:
	py/visdom/static/js/main.js
@clement-masson
Copy link
Contributor Author

Hi.
I merged my commit with the ones added yesterday.
Is it ok like this or do I need to create a new PR ?

@JackUrb
Copy link
Contributor

JackUrb commented Jul 18, 2018

Alright it's good to go now. Thanks for the contribution!

Copy link

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@JackUrb is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

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.

None yet

4 participants