DrawPanel Feature: Grouping Shapes

[ganimtron-10]

This feature adds grouping functionality to the shapes present in the DrawPanelUI.
PR #623 needs to be merged before this.

To group shapes:

• Go to selection mode
• Hold the Ctrl Key
• Select the shape you want to group together using left mouse click
• If you want to ungroup the shape, just select the shape again and it will be automatically deselected.

Features:

• Translation of all the shapes
• Rotation of all the shapes

Demo:

[pep8speaks]

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-09-12 15:09:33 UTC

[m-agour]

Hello @ganimtron-10,
Not sure if you noticed or not but all tests are failing for some reason.

[nasimanousheh]

@ganimtron-10 Thank you for updating the PR with grouping the shapes functionality.
When another mode (for example delete) is selected, the white borders around the shapes and the rotation slide should get disappeared.

[ganimtron-10]

I tried various ways to find the issue and found out Disk2D's rotation was causing this failure so I removed that part and rerecorded the test but still as we can see the tests are still failing. Mohamed also tried running this test on his Windows, there the tests are passing sucessfully. Also if we see the failing pattern it isn't consistent in some cases only rotation test is failing in some the grouping test is failing.

[ganimtron-10]

Hello @skoudoro @Garyfallidis ,
According to the suggestions, I tried using the Button2D instead of the RingSlider to rotate the shapes, and maybe my guess was correct about the slider (that it was causing the tests to fail, but I can't validate it) because after these changes all the test is passing.
This is the link to see the changes and test results: ganimtron-10#8

[skoudoro]

Thank you for going deeper to this. So We need to look RingSlider only. We might miss something that will bother us in the future. Good investigation @ganimtron-10

[codecov]

Codecov Report

Merging #653 (2cd1b7a) into master (8710a65) will decrease coverage by 0.36%.
The diff coverage is 0.00%.

❗ Current head 2cd1b7a differs from pull request most recent head 874e813. Consider uploading reports for the commit 874e813 to get more accurate results

@@            Coverage Diff             @@
##           master     #653      +/-   ##
==========================================
- Coverage   50.47%   50.11%   -0.36%     
==========================================
  Files         126      120       -6     
  Lines       28299    28311      +12     
  Branches     3034     3049      +15     
==========================================
- Hits        14284    14189      -95     
- Misses      13536    13650     +114     
+ Partials      479      472       -7     

Impacted Files	Coverage Δ
fury/animation/interpolator.py	0.00% <0.00%> (ø)
fury/animation/tests/test_timeline.py	0.00% <0.00%> (ø)
fury/animation/timeline.py	0.00% <0.00%> (ø)
fury/colormap.py	0.00% <0.00%> (ø)
fury/gltf.py	0.00% <0.00%> (ø)
fury/shaders/tests/test_base.py	0.00% <0.00%> (ø)
fury/transform.py	0.00% <0.00%> (ø)
fury/ui/containers.py	0.00% <0.00%> (ø)
fury/ui/core.py	0.00% <0.00%> (ø)
fury/ui/elements.py	0.00% <0.00%> (ø)
... and 35 more

[Garyfallidis]

@ganimtron-10 I want to merge this but I saw that sometimes the "test_ui_draw_panel_grouping" is failing. Can you suggest how to move forward? Should we ignore the failing because this will come together with another PR? I am not sure how to proceed. Please advise.

[ganimtron-10]

Hello @Garyfallidis ,
Currently, I am digging deep into the event propagation and finding out why the rotation function is called multiple times when clicked once. I think I would need more time to look into it.

As you suggest there are mainly two ways to merge this PR:

1. Skip these tests and create an issue regarding the same and fix it as soon as possible (but then these features remain untested and may cause some issues in future).
2. Second would be to replace the current rotation slider with two button to manually rotate the shape by some fixed step value on both clockwise as well as counter clockwise direction, but in this case the flexibility of rotation reduces a lot. (I tried integrating spinbox UI with this, but still theres some kind of error in event propogation which again hamper the working.)

Let me know, how should I go ahead with it!!

[Garyfallidis]

You do not need initialize here?

[ganimtron-10]

Nope, it is internally called in the show_manager.start()

[Garyfallidis]

Let's investigate deeper. Neither 1 or 2 are viable long term solutions.

[ganimtron-10]

I tried investigating the issues but was unable to narrow it down.

I can show that the code repeats itself from the propagate_event but am unable to find what cause it to start and how it automatically stops after an even number of iterations.

[skoudoro]

Thank you for your feedback @ganimtron-10.

We are also still investigating, and I wonder if it is not the add_callback function is the customInteractor that is failing sometimes.

Investigations still in process. Thank you for still looking into it

[Garyfallidis]

@ganimtron-10 please update these PR.