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

Creating DrawPanel UI #599

Merged
merged 48 commits into from Jun 30, 2022
Merged

Conversation

ganimtron-10
Copy link
Contributor

@ganimtron-10 ganimtron-10 commented Jun 9, 2022

This UI would help us to create various shapes and transform them.

Currently, it supports

  • Creation of
    • Line
    • Circle
    • Quad
  • Selection (Translation)
  • Deletion
  • Shapes are perfectly clamped

Demo

python_Dg9yGkR9DT

@ganimtron-10 ganimtron-10 marked this pull request as draft June 9, 2022 06:39
@codecov
Copy link

codecov bot commented Jun 9, 2022

Codecov Report

Merging #599 (24412af) into master (01a7ce2) will increase coverage by 0.04%.
The diff coverage is n/a.

❗ Current head 24412af differs from pull request most recent head 18fb1b8. Consider uploading reports for the commit 18fb1b8 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #599      +/-   ##
==========================================
+ Coverage   87.93%   87.98%   +0.04%     
==========================================
  Files          62       62              
  Lines       13126    13342     +216     
  Branches     1322     1355      +33     
==========================================
+ Hits        11543    11739     +196     
- Misses       1206     1216      +10     
- Partials      377      387      +10     
Impacted Files Coverage Δ
fury/fury/stream/tools.py 90.94% <0.00%> (-0.22%) ⬇️
fury/fury/ui/elements.py 87.64% <0.00%> (+0.34%) ⬆️
fury/fury/ui/tests/test_elements.py 74.76% <0.00%> (+0.82%) ⬆️
fury/fury/data/fetcher.py 72.66% <0.00%> (+2.66%) ⬆️

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 @ganimtron-10,

Thank you for this feature. This is really nice! see below for some comments.

it would be also good :

  • if you could add the erase functionality
  • To add an option to tell if DrawPanel is movable or not
  • When we start, we see mode selection, but nothing happens when we click the drawer panel, it is confusing. it will be good to do something.
  • it would be good to use the on/off capabilities of button2D to know which tool we are using

docs/tutorials/02_ui/viz_drawpanel.py Outdated Show resolved Hide resolved
fury/ui/elements.py Outdated Show resolved Hide resolved
fury/ui/elements.py Outdated Show resolved Hide resolved
fury/ui/elements.py Outdated Show resolved Hide resolved
fury/ui/elements.py Outdated Show resolved Hide resolved
fury/ui/elements.py Outdated Show resolved Hide resolved
fury/ui/elements.py Outdated Show resolved Hide resolved
fury/ui/elements.py Outdated Show resolved Hide resolved
fury/ui/elements.py Outdated Show resolved Hide resolved
fury/ui/elements.py Outdated Show resolved Hide resolved
@m-agour
Copy link
Contributor

m-agour commented Jun 9, 2022

Hello @ganimtron-10 ,
Great job you did there! It's really nice to draw primitives like that, maybe for describing a scene or draw an arrow to point at something and a lot of other useful applications.

  • I experimented with the panel, and I have found some minor glitches:
  1. when I click in place in drawing mode, it draws a time point which does not get rendered until I draw a big shape or force render the scene somehow (I'm not sure if it should be allowed to draw shapes in place unless I'm drawing a point).
2022-06-09.23-26-21.mp4
  1. The above issue also triggered the below runtime warning:
 ..\fury\fury\ui\elements.py:3149: RuntimeWarning: divide by zero encountered in long_scalars
  self.rotate_line(deg=np.arctan(size[1]/size[0]))
  • Another minor note is there should be a separate selection mode and selecting shapes while drawing should be disabled or handled in another way, because in the current state I can't draw shapes above shapes.

Copy link
Member

@xtanion xtanion left a comment

Choose a reason for hiding this comment

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

Hi @ganimtron-10, This feature is really nice and everything is working fine. There are a few runtime warnings I encountered, Look at my comment below.
Thanks.

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

pep8speaks commented Jun 10, 2022

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-06-30 16:10:30 UTC

@ganimtron-10
Copy link
Contributor Author

Thank You @m-agour and @xtanion for your wonderful review!!

fury/data/fetcher.py Outdated Show resolved Hide resolved
@ganimtron-10
Copy link
Contributor Author

Hello @Garyfallidis @nasimanousheh @skoudoro ,
I have updated the DrawPanel and DrawShape UI, due to which the shapes are now restricted to remain inside the canvas.
PTAL.

The tests are failing because I have separated the code for the fetcher function which fetches new icons required for the DrawPanel UI.

Thanks!

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 @ganimtron-10,

This first version looks really good.

we can not test this tutorial because the fetcher is missing. Since the fetcher PR has been merged, Can you merge master or rebase this PR?

After that, and few other fixes, it should be ready to be merged. Thanks for this clean work

fury/ui/elements.py Outdated Show resolved Hide resolved
self.canvas.background.on_left_mouse_button_pressed = self.left_button_pressed
self.canvas.background.on_left_mouse_button_dragged = self.left_button_dragged

mode_data = {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you note this somewhere as a future new feature

}

padding = 5
mode_panel_size = (len(mode_data) * 35 + 2 * padding, 40)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you note this somewhere as a future new feature

fury/ui/elements.py Outdated Show resolved Hide resolved
fury/ui/elements.py Show resolved Hide resolved
fury/ui/elements.py Show resolved Hide resolved
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.

ok, LGTM! thanks @ganimtron-10!

This is ready to go. I will wait for another review until Wednesday. if no other review, I will go ahead and merge it.

I found one small bug but I think, this fix or any new features should be done in a new PR.

Good job!

fury/ui/elements.py Show resolved Hide resolved
@nasimanousheh
Copy link
Contributor

Hi @ganimtron-10. Great job! you fixed the boundary issue and by moving the shape, it doesn't go out of the canvas boundary.
I found two bugs in your recent changes:

  • I can not move the shape with one click. I need to double click on the shape to move it.
  • When I double click on the shape, without moving the mouse, the shape jumps to other position. As you see here:

fail

@ganimtron-10
Copy link
Contributor Author

Hello @nasimanousheh ,

  • I can not move the shape with one click. I need to double click on the shape to move it.

When I use my external mouse single click with the dragging works pretty fine, but when I use my trackpad mouse I have to double click and then drag.

  • When I double click on the shape, without moving the mouse, the shape jumps to other position. As you see here:

fail

There was an issue with the positioning of the element in the panel which is now fixed using relative positioning.

PTAL.

Thanks!

fury/ui/elements.py Outdated Show resolved Hide resolved
fury/ui/elements.py Show resolved Hide resolved
fury/ui/elements.py Show resolved Hide resolved
fury/ui/elements.py Show resolved Hide resolved
fury/ui/elements.py Show resolved Hide resolved
@Garyfallidis
Copy link
Contributor

Great job @ganimtron-10 ! Merging. Bring forth the titans aka next PRs!

@Garyfallidis Garyfallidis merged commit f82e195 into fury-gl:master Jun 30, 2022
@ganimtron-10 ganimtron-10 deleted the DrawPanel_UI branch August 7, 2022 07:57
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

7 participants