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

Image element in dipy.viz.ui #1522

Merged
merged 7 commits into from Jun 15, 2018

Conversation

Projects
8 participants
@karandeepSJ
Copy link
Contributor

karandeepSJ commented May 15, 2018

Implemented a new UI element that is capable of displaying an image when provided with the image path.
Tests written are passing.
Currently supports JPEG/JPG and PNG images.

Features to be added

  • Dragging and Resizing by mouse
  • Browse file system to select and change image
@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented May 15, 2018

Hello @karandeepSJ, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on June 14, 2018 at 02:54 Hours UTC

@skoudoro skoudoro added the gsoc2018 label May 15, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 15, 2018

Codecov Report

Merging #1522 into master will increase coverage by 0.17%.
The diff coverage is 86.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1522      +/-   ##
==========================================
+ Coverage   87.42%   87.59%   +0.17%     
==========================================
  Files         246      246              
  Lines       31458    31359      -99     
  Branches     3425     3412      -13     
==========================================
- Hits        27502    27469      -33     
+ Misses       3144     3080      -64     
+ Partials      812      810       -2
Impacted Files Coverage Δ
dipy/viz/tests/test_ui.py 87.33% <75%> (-0.17%) ⬇️
dipy/viz/ui.py 90.01% <88.31%> (-0.89%) ⬇️
dipy/viz/interactor.py 98.12% <0%> (-0.04%) ⬇️
dipy/tracking/streamline.py 96.06% <0%> (+28.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bfa3ac...3336f73. Read the comment docs.

@ranveeraggarwal

This comment has been minimized.

Copy link
Member

ranveeraggarwal commented May 16, 2018

We have some pending changes in #1492 which would change the base UI class. Therefore, we'll need some changes to this PR when #1492 is merged.
I am currently reviewing it - we should be able to merge it by this week.
cc: @skoudoro @Garyfallidis

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented May 16, 2018

The element button2d accept an image/icon so I wonder whether button2d will use this element?

I will review this PR ASAP

@ranveeraggarwal

This comment has been minimized.

Copy link
Member

ranveeraggarwal commented May 16, 2018

That's a good idea. Once this is merged, we can look at modifying the Button2D element to use this.

png.SetFileName(imgPath)
png.Update()
img = png.GetOutput()
elif imgPath.split(".")[-1] in ["jpg", "jpeg", "JPG", "JPEG"]:

This comment has been minimized.

@nilgoyette

nilgoyette May 18, 2018

Contributor

Putting imgPath.split(".")[-1] in an ext variable would be better.

Also, you should use lower() and compare the extension only to lower-case strings. A windows user could use a valid PnG as input.

self.texture_polydata.GetPointData().SetTCoords(tc)

texture_mapper = vtk.vtkPolyDataMapper2D()
if major_version <= 5:

This comment has been minimized.

@nilgoyette

nilgoyette May 18, 2018

Contributor

I know it's not your fault, but major_version is a terrible name. The major version of what? @skoudoro do you have an opinion on this? Should we rename this? And maybe create a function with the patterns

if version SetInput else SetInputData

because it's used 4 times only in this file. We might also wonder if there's these still some people using vtk5. It's been 5 years since vtk6.

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Jun 10, 2018

Member

+1 @karandeepSJ please look into this.

This comment has been minimized.

@dmreagan

dmreagan Jun 11, 2018

Contributor

This looks like a good use for the viz utility function set_input

This comment has been minimized.

@karandeepSJ

karandeepSJ Jun 11, 2018

Author Contributor

I'll create a separate PR for this.

@ranveeraggarwal

This comment has been minimized.

Copy link
Member

ranveeraggarwal commented Jun 3, 2018

@karandeepSJ now that Marc's PR is merged, can you refactor this code to go with the new base UI element? And #1534 too.

@karandeepSJ karandeepSJ force-pushed the karandeepSJ:ImageHolder branch from 66e150f to 703a112 Jun 5, 2018

@dmreagan dmreagan added this to PR needs a review in Viz Module Jun 7, 2018

@@ -2161,3 +2163,185 @@ def handle_move_callback(self, i_ren, obj, slider):
self.move_handle(click_position=click_position)
i_ren.force_render()
i_ren.event.abort() # Stop propagating the event.


class ImageHolder(UI):

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Jun 10, 2018

Member

ImageHolder2D probably be a better name.

This comment has been minimized.

@dmreagan

dmreagan Jun 11, 2018

Contributor

I don't love "holder." Maybe ImageContainer2D?

"""

def __init__(self, imgPath, position=(0, 0), size=(100, 100)):

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Jun 10, 2018

Member

Parameter names are snake_cased. Replace imgPath with img_path

Width and height in pixels of the image.
"""
super(ImageHolder, self).__init__(position)
self.img = self._build_image(imgPath)

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Jun 10, 2018

Member

Add a description of this attribute to the docstring.

self.texture_polydata.GetPointData().SetTCoords(tc)

texture_mapper = vtk.vtkPolyDataMapper2D()
if major_version <= 5:

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Jun 10, 2018

Member

+1 @karandeepSJ please look into this.

"""
ren.add(self.actor)

def resize(self, size):

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Jun 10, 2018

Member

I think this could be refactored to just use size as a property. Let me know what you think @karandeepSJ

This comment has been minimized.

@karandeepSJ

karandeepSJ Jun 11, 2018

Author Contributor

Size is not an attribute in the UI class or in any other classes. It would be best to maintain that uniformity and keep it as resize()

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Jun 11, 2018

Member

You're correct. The way I thought of this is that size is an important attribute for some elements.
For instance, in the TextBlock/TextBox, text is a major attribute and therefore we make that a property. Why not do the same for size?
Maybe we can keep it the way it is for now and later refactor all elements if it makes sense.

img : imageDataGeometryFilter
"""
if major_version <= 5:

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Jun 10, 2018

Member

Same as above, use something like vtk_major_version instead of major_version

This comment has been minimized.

@karandeepSJ

karandeepSJ Jun 11, 2018

Author Contributor

Should I change this for all classes?

This comment has been minimized.

@ranveeraggarwal

ranveeraggarwal Jun 11, 2018

Member

Sure, that would be great

This comment has been minimized.

@karandeepSJ

karandeepSJ Jun 11, 2018

Author Contributor

I'll do it in a separate PR using the set_input function from utils

@dmreagan
Copy link
Contributor

dmreagan left a comment

The code looks solid, but please add an example to viz_ui.py.



class ImageHolder(UI):
""" A 2D container to hold an image and is of type vtkTexturedActor2D.

This comment has been minimized.

@dmreagan

dmreagan Jun 11, 2018

Contributor

I don't think it's necessary to say what type of actor is used. Just leave it at "A 2D container to hold an image."

@dmreagan

This comment has been minimized.

Copy link
Contributor

dmreagan commented Jun 13, 2018

For a future PR: it would be nice to have something like CSS's object-fit to handle mismatches between the image size and the container size.

@karandeepSJ karandeepSJ force-pushed the karandeepSJ:ImageHolder branch from 1121de4 to 3336f73 Jun 14, 2018

@Garyfallidis Garyfallidis merged commit 7b3b633 into nipy:master Jun 15, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

Viz Module automation moved this from PR needs a review to Done Jun 15, 2018

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

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