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

[Cropping] reset box if tlbx is switched to another & add reset button #826

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mathildemerle
Copy link

This PR is about the Cropping bug in https://github.com/Inria-Asclepios/music/issues/1056

The problem was that there was no way to reset a Cropping box after the disappearance of the box, if the user moves it outside of the view for instance.

This PR adds a reset of the box if you switch to an other toolbox, change data, apply data (the new data is applied in the view). In pipeline it's possible to Reset the toolbox directly with the pipeline button.

Concerning the orientation problem on mac, i had it once and couldn't redo it, so i couldn't really work on that qt or vtk problem, but resetting the toolbox solves the problem.

Ⓜ️

@mathildemerle mathildemerle added this to the v4.0 milestone Feb 29, 2024
@fcollot
Copy link

fcollot commented Mar 1, 2024

If you apply the crop, then move the box and reset, the box disappears and from then on apply causes "Cropping: This action failed (undefined error)" and will not work anymore (even when closing the view and adding new data).

@mathildemerle
Copy link
Author

@fcollot i don't succeed to redo your error. Did you change the orientation or move the box without resetting, or do something in addition ? Indeed if i move the box outside the view and apply i've got the error (it does that even without this PR), but resetting works. I'll continue to test

@mathildemerle
Copy link
Author

Are you talking about the new Reset button in the toolbox, or the Reset button in a the pipeline toolbox ?

@fcollot
Copy link

fcollot commented Mar 1, 2024

The toolbox button. On every data if I apply the crop, then move the cropping box and click reset, it causes the bug.

@mathildemerle
Copy link
Author

It's interesting because Kévin had some disappearance of the box changing the orientation, that i had only once and could not reproduce. It can be the same underlying problem

@fcollot
Copy link

fcollot commented Mar 1, 2024

I don't think it's related. From looking at the code there are some issues I believe could cause this.

@mathildemerle
Copy link
Author

(i just removed a duplicated line, render was done also in updateBorderWidgetIfVisible)
If you see some problems do not hesitate to explain them, because i can't reproduce the bug

@fcollot
Copy link

fcollot commented Mar 1, 2024

(that render line was one of the things I was mentioning, but I wanted to do a few tests first)
I still have this bug (for example when I move the box, then reset, then apply and reset again). I can reproduce this bug every time by simply doing a combination of move + reset -> apply -> reset (or a different order, as long as there is a reset after moving). Since it breaks the toolbox permanently (until closing the app) I can't validate this.

@mathildemerle
Copy link
Author

Yes, however since Kévin had the same bug just changing the orientation, can you test with or without this PR? Because changing the orientation and resetting lead to the same updateBorderWidgetIfVisible() line, from reactToOrientationChange() or resetBoxPlace()

@fcollot
Copy link

fcollot commented Mar 1, 2024

Since it is not possible to reproduce my test without this PR I suppose you are talking about the pipeline reset button (which is entirely different since it destroys and recreates the toolbox and therefore everything inside it including the border widget). I just tested this multiple times, moving the box in different orientations, resetting, clicking next then coming back etc. without any bug.

@mathildemerle
Copy link
Author

I was talking about the orientations in the "View settings" toolbox. Kévin changed the orientation of its view and the box disappeared, even without resetting the toolbox through the pipeline Reset button. That was the main problem i was trying to solve at the beginning, but since i did not have this bug on my mac, i wanted to give the opportunity to reset the box.
In fact resetting the box in the toolbox in the workspace is needed when the user moves the box outside the view.
However thx to your test it seems that it does not bring back to life a box if it disappeared completely (maybe vtkBorderWidget or vtkBorderRepresentation crashes, i'm not sure)

@mathildemerle
Copy link
Author

I removed this one from the 4.0 milestone. It's a feature that could be interesting later

@mathildemerle mathildemerle removed this from the v4.0 milestone Mar 8, 2024
@mathildemerle
Copy link
Author

Information: a new message from Ludovic (radio operator):

J'ai eu un bug sur l'étape de cropping, lorsque je changeais de vue dans view settings, il m'enlevait le rectangle vert et pas moyen de le retrouver ni de faire next. J'ai recommencé plusieurs fois avec reset ou nouveau pipeline, même résultat jusqu'à un plantage total puis tout à fonctionné normalement.

The underlying problem is still on 4.0.7 on macOS. The cropping box could disappear sometimes changing the orientation. It still do not happen on Ubuntu from what i see. I am testing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants