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

[ui] Improve "Clear Images" action's behaviour and performance #1897

Merged
merged 2 commits into from
Mar 3, 2023

Conversation

cbentejac
Copy link
Contributor

@cbentejac cbentejac commented Feb 15, 2023

Description

This PR modifies the behaviour of the "Clear Images" menu action as follows:

  • It is now executed on the Python side instead of the QML side, which makes it more than 4x faster:
    • For 500 PNG images, it used to take ~38s, it now takes less than 0.5s.
    • For 1000 PNG images, it used to take ~2m39, it now takes 1s.
  • "Clear Images" now only clears the images for the active group (meaning the active CameraInit node), instead of clearing all the images for all groups (all CameraInit nodes).
  • When using "Clear Images", the "Undo" stack used to be filled with individual intrinsics and viewpoints images. Calling "Clear Images" when 500 images were in the gallery was thus requiring at least 500 calls to "Undo" to be fully reverted. It is now considered as a single action, meaning a single call to "Undo" is enough to recover all the cleared images.

A "Clear All Images" action has been added in the "Advanced" sub-menu to clear the images of all the groups.

Tooltips have been added over both "Clear Images" and "Clear All Images" to make the distinction clear for users.

Features list

  • Execute "Clear Images" on the Python side rather than the QML side;
  • Make "Clear Images" remove only the images of the current CameraInit node;
  • Add a "Clear All Images" action in the "Advanced" sub-menu to remove the images of all the CameraInit nodes.

The "Clear Images" action was performed solely on the QML side, updating
the graph with every intrinsics removal, which considerably slowed down
the whole process.

The QML action now calls a slot on the Python side, which can disable
the graph updates at every modification, thus greatly improving the speed
of the process.

As a point of comparison, clearing 511 PNG images took approximately 38s
on the QML side, against 8s on the Python side. Clearing 1000 PNG images
took 2min39 on the QML side, against 32s on the Python side.
@mugulmd
Copy link
Contributor

mugulmd commented Feb 16, 2023

I'm not sure we should be using "Ctrl-D" and "Ctrl-Shift-D" for clearing images since the letter D is usually reserved for duplicating. If that doesn't create any conflict I would rather use the letter X instead, what do you think ?

@fabiencastan
Copy link
Member

We don't need to do that very often, so I think we can just remove the shortcut.

@cbentejac
Copy link
Contributor Author

We don't need to do that very often, so I think we can just remove the shortcut.

I dropped the commit that added the shortcuts.

@cbentejac cbentejac marked this pull request as draft February 16, 2023 15:34
@cbentejac cbentejac marked this pull request as ready for review February 20, 2023 15:28
@cbentejac cbentejac force-pushed the dev/improveClearImages branch 2 times, most recently from 626bd25 to df26753 Compare February 20, 2023 15:45
meshroom/ui/commands.py Outdated Show resolved Hide resolved
…viour

"Clear Images" used to clear the intrinsics and viewpoints for all the
existing CameraInit nodes in the graph. There was no-user friendly way
to clear the images of a specific CameraInit node.

This commit modifies the behaviour of "Clear Images" so that it only
deletes the intrinsics and viewpoints of the current CameraInit. A new menu
action, "Clear All Images", is added in the "Advanced" sub-menu, and
deletes all the intrinsics and viewpoints for all the CameraInit nodes.

The way images are cleared is also modified: instead of removing the
intrinsics and viewpoints attributes, they are reset. To be undone
properly, a corresponding "ClearImagesCommand" has been added. This offers
a great performance increase (clearing 1000 images now takes 1s).

Tooltips have been added to make the distinction clearer for users.
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.

3 participants