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

Adding Double Click Callback #231

Merged
merged 31 commits into from Jun 10, 2020
Merged

Conversation

Nibba2018
Copy link
Member

Referencing #226 , Adding Double Left Click Callback.
I will be adding examples & unittests soon.

Let me know if I am doing it wrong.
Thank You.

@codecov
Copy link

codecov bot commented Apr 3, 2020

Codecov Report

Merging #231 into master will increase coverage by 0.02%.
The diff coverage is 89.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #231      +/-   ##
==========================================
+ Coverage   88.46%   88.49%   +0.02%     
==========================================
  Files          17       17              
  Lines        4386     4423      +37     
  Branches      556      569      +13     
==========================================
+ Hits         3880     3914      +34     
  Misses        366      366              
- Partials      140      143       +3     
Impacted Files Coverage Δ
fury/ui.py 86.13% <0.00%> (-0.16%) ⬇️
fury/interactor.py 96.92% <97.22%> (+0.53%) ⬆️

@Nibba2018
Copy link
Member Author

@skoudoro I need some suggestions here...

According to this example they have overridden the OnLeftButtonDown by inheriting from vtkInteractorStyleTrackballCamera class. But the rest of the callbacks are implemented in a different way. So should I inherit both vtkInteractorStyleUser and vtkInteractorStyleTrackballCamera and implement the double click? or should I create a separate class for it?

I am also unable to implement my double click callback on an actor hence not being able to test my implementation. I need some help here with that.

Thanks.

@skoudoro skoudoro added this to the v0.6.0 milestone Apr 11, 2020
Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @Nibba2018,

So should I inherit both vtkInteractorStyleUser and vtkInteractorStyleTrackballCamera and implement the double click? or should I create a separate class for it?

inherit from vtkInteractorStyleUser so inherit from the current class

I am also unable to implement my double click callback on an actor hence not being able to test my implementation. I need some help here with that.

There are many ways to do it and it is not trivial. In your first test, you should do every in on_left_button_down. Then, when it works you can create a new custom event and use the invokeevent function. I will try to give you more details later this weekend but you can update this first. Look at all existing vtk event and How to create a custom event.

fury/interactor.py Outdated Show resolved Hide resolved
fury/interactor.py Outdated Show resolved Hide resolved
fury/interactor.py Outdated Show resolved Hide resolved
fury/interactor.py Outdated Show resolved Hide resolved
@pep8speaks
Copy link

pep8speaks commented Apr 11, 2020

Hello @Nibba2018! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 151:24: E127 continuation line over-indented for visual indent

Comment last updated at 2020-06-09 22:07:04 UTC

@Nibba2018
Copy link
Member Author

Here's a test that I am running to test double click implementation where I change the color of the cube when double clicked on it.

from fury import actor, window, interactor
import numpy as np

cube = actor.cube(np.random.rand(1, 3), np.random.rand(1,3), (1,1,1), heights=np.random.rand(5))

def change_color(iren, obj):
	obj.SetColor(np.random.rand(3))
	iren.force_render()

interactor_style = interactor.CustomInteractorStyle()
interactor_style.add_active_prop(cube)
interactor_style.add_callback(cube, "LeftButtonPressEvent", change_color)

current_size = (800, 800)
showm = window.ShowManager(size=current_size, title="Double Click Test")
showm.scene.add(cube)
showm.start()

Apparantly I am having an error message being printed which I am not able to understand why. The output:

Initial state: (390, 379)
Final state: (390, 379)
Initial state: (390, 379)
Final state: (390, 379)
Double Clicked...
interactor is none
event name is LeftButtonPressEvent

interactor is none
event name is LeftButtonPressEvent

@skoudoro
Copy link
Contributor

  1. Can you differentiate LeftButtonPressEvent and DoubleClick? For example, on DoubleClick, switch color between red and blue, for a single click switch between yellow and purple.

  2. You do not need interactor_style = interactor.CustomInteractorStyle(). This is created by ShowManager and if you want to access it, you just need to do showm.iren. This will avoid your error

  3. showm.iren.add_callback (cube, ...)

nice progress!

@Nibba2018
Copy link
Member Author

Hello @skoudoro , I tried showm.iren.add_callback(...) but it gives me an Attribute Error

Traceback (most recent call last):
  File "double_click.py", line 13, in <module>
    showm.iren.add_callback(cube, "LeftButtonPressEvent", change_color)
AttributeError: 'vtkRenderingOpenGL2Python.vtkXRenderWindowInteract' object has no attribute 'add_callback'

fury/interactor.py Outdated Show resolved Hide resolved
@skoudoro
Copy link
Contributor

I tried showm.iren.add_callback(...) but it gives me an Attribute Error

ok, need to check, maybe it is showm.style.add_callback instead

@Nibba2018
Copy link
Member Author

Hello @skoudoro , I am successfully able to detect and differentiate between single clicks & double clicks. But I am unable to understand how to abort or stop the propagation of the previously initiated single click event. Can you give me some idea where I should look for a solution... Thank you.

@Nibba2018
Copy link
Member Author

Nibba2018 commented Apr 19, 2020

Also, is the line self.AddObserver("LeftButtonDoubleClickEvent", self._process_event) sufficient enough to create a new event for double click? or do I need to do something else?

@skoudoro
Copy link
Contributor

Hello @skoudoro , I am successfully able to detect and differentiate between single clicks & double clicks. But I am unable to understand how to abort or stop the propagation of the previously initiated single click event. Can you give me some idea where I should look for a solution... Thank you.

Hi @Nibba2018, That's good news! To stop the propagation,you need to use self.event.abort().
But, to use this method at the right place can be tricky, I need to go deeper into it.

Also, is the line self.AddObserver("LeftButtonDoubleClickEvent", self._process_event) sufficient enough to create a new event for double click? or do I need to do something else?

No, it is not because this custom event is never invoked. We need to create a custom event named LeftButtonDoubleClickEvent and then invoke it on our interactor. You will have to look at VTK examples to see how to manage a new custom event.

@Nibba2018
Copy link
Member Author

Nibba2018 commented Apr 22, 2020

Hello @skoudoro , I did some research and have come up with the following conclusions:

  • The custom event can be implemented by subclassing vtkCommand.
  • I was also able to find an example related to vtkCommand, but I am not sure how it works or how it is being implemented. I will be looking into it further.
  • I also found some contradictions regarding subclassing vtkCommand here, but I am not sure whether it's outdated or not.

I will try implementing vtkCommand subclasses and see if it works.

@skoudoro
Copy link
Contributor

skoudoro commented Apr 22, 2020

Hi @Nibba2018,

Thank you for your research and all this information. You should look at this nice example too.
Basically, it seems that you need to follow this step:

  1. create DoubleClickEvent in the init of our custom interactor style : self.DoubleClickEvent = vtk.vtkCommand.UserEvent+1
  2. invoke this event on the leftclick fonction when you detect it: self.invokeEvent(self.DoubleClickEvent, 'DoubleClickEvent')
  3. connect DoubleClickEvent with your double click function.

Does it make sense?

@Nibba2018
Copy link
Member Author

Thank you @skoudoro , It makes it very clear. I will implement this right away.

@Nibba2018
Copy link
Member Author

Nibba2018 commented Apr 23, 2020

@skoudoro I am using this following for testing:

from fury import actor, window, interactor
import numpy as np

cube = actor.cube(np.random.rand(1, 3), np.random.rand(1,3), (1,1,1), heights=np.random.rand(5))

def single_click(iren, obj):
	print("Single Click")
	iren.force_render()

def double_click(iren, obj):
	print("Double Click")
	iren.force_render()

current_size = (800, 800)
showm = window.ShowManager(size=current_size, title="Double Click Test")
showm.scene.add(cube)
showm.style.add_callback(cube, "LeftButtonPressEvent", single_click)
showm.style.add_callback(cube, "DoubleClickEvent", double_click)
showm.start()

The changes are able to detect and differentiate clicks but is not able to propagate the Double Click Event Callback.

@Nibba2018
Copy link
Member Author

@skoudoro , I ran the debugger and found that during single clicks, the control moves from self.propagate to prop.InvokeEvent and then to _add_callback function. But in the case of Double Clicks, the control doesn't go to _add_callback after the prop.InvokeEvent has been executed...

@skoudoro
Copy link
Contributor

Interesting, I have no idea at this point, I need to go deeper.

Maybe @MarcCote have an idea about the DoubleClik

@skoudoro
Copy link
Contributor

skoudoro commented Apr 24, 2020

I suppose you are missing a self.InvokeEvent(...) to propagate the event to himself (the interactor)

prop.InvokeEvent propagates the event to all dependent actor

So you need both @Nibba2018

@Nibba2018
Copy link
Member Author

Nibba2018 commented Apr 24, 2020

@skoudoro , Initially I tried with self.InvokeEvent but it didn't work, so I thought I should be using prop.InvokeEvent instead as it was previously implemented in self.propagate_event.

This time I will try using both.

Copy link
Contributor

@MarcCote MarcCote left a comment

Choose a reason for hiding this comment

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

Hi @Nibba2018, thank you so much for taking a stab at this. Sorry for being late to the party. See my comments in the code to fix your double click issue.

On a side note, the expected behavior of a double click event is:

  • Two consecutive clicks on the same object/prop within some time interval.

At the moment, I see that the second click has to be in some radius of the first one. What's the expected behavior when the two clicks are not on the same prop/object but still within the predefined radius?

Regarding the "within a time interval", I guess we can do it in another PR.

fury/interactor.py Outdated Show resolved Hide resolved
fury/interactor.py Outdated Show resolved Hide resolved
fury/interactor.py Outdated Show resolved Hide resolved
fury/interactor.py Outdated Show resolved Hide resolved
@Nibba2018
Copy link
Member Author

Hello @MarcCote , thank you for the review I will apply the changes and will let you know if I have any questions.

@Nibba2018
Copy link
Member Author

Nibba2018 commented May 14, 2020

@MarcCote Thank you for the suggestions. The double click callback now triggers normally. Regarding the double click behavior, Yes it only detects double clicks if the second click is within a certain radius of the first click irrespective of the prop. I will add some changes to in order to detect whether the clicks are on the same prop or not.

@Nibba2018 Nibba2018 closed this May 14, 2020
@Nibba2018 Nibba2018 reopened this May 14, 2020
@Nibba2018 Nibba2018 changed the title [WIP] Adding Double Click Callback Adding Double Click Callback May 15, 2020
@Nibba2018
Copy link
Member Author

Nibba2018 commented May 15, 2020

Hello @MarcCote , looks like single clicks arent working the way they are supposed to and hence causing tests to fail. Failed tests start from test_ui_line_slider_2d_horizontal_bottom.

@Nibba2018 Nibba2018 requested a review from MarcCote May 15, 2020 14:20
@skoudoro
Copy link
Contributor

Thank you for the rebase @Nibba2018.

Any additional comments? I will merge this by the end of the day.

Copy link
Contributor

@MarcCote MarcCote left a comment

Choose a reason for hiding this comment

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

LGTM

@skoudoro
Copy link
Contributor

Thank you @MarcCote and @Nibba2018 for this nice work. merging

@skoudoro skoudoro merged commit 41d8ca3 into fury-gl:master Jun 10, 2020
@skoudoro skoudoro linked an issue Jun 10, 2020 that may be closed by this pull request
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.

Adding DoubleClick event
4 participants