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

Show generated images in 2D viewer when double-clicking on node #1776

Merged
merged 30 commits into from
Oct 19, 2022

Conversation

mugulmd
Copy link
Contributor

@mugulmd mugulmd commented Sep 21, 2022

Description

When double-clicking on the PrepareDenseScene, DepthMap or DepthMapFilter node, the generated images (undistorted images, depth maps, sim maps) corresponding to the currently selected viewpoint will automatically appear in the 2D viewer.

This behaviour can be applied on other nodes that generate images with the following changes in the node description :

  • if it doesn't already exist, add an output File attribute
  • set its 'semantic' field to 'image'
  • set its value to a string describing the output image files, using the <VIEW_ID> macro to represent the corresponding viewId
    (see DepthMap.py for an example).

Implementation remarks

  • Viewer2D.qml: the ComboBox doesn't represent an image type anymore but rather the selected output attribute name (corresponding to the images to be visualized)
  • Viewer2D.qml: we now hold a reference to the node being displayed
  • onCurrentItemChanged in WorkspaceView.qml has been replaced with onSelectedViewIdChanged in Viewer2D.qml as it removes a dependency between 2 UI elements with a more manageable dependency between backend and frontend

@mugulmd mugulmd self-assigned this Sep 21, 2022
@mugulmd mugulmd marked this pull request as ready for review September 21, 2022 13:02
@fabiencastan fabiencastan added this to the Meshroom 2022.1.0 milestone Sep 21, 2022
viewer2D.displayedNode = node;

// 3D viewer
for(var i=0; i < node.attributes.count; ++i)
Copy link
Member

Choose a reason for hiding this comment

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

could be good to add semantic for 3D too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should make a separate issue to normalize the use of the semantic field (so we can anticipate problems that may appear on the long term before using this concept in too many places of the code), and at this point yep sure it'll be a good idea to add 3D to semantics. What do you think ?

@fabiencastan
Copy link
Member

Need to adjust size for the combobox width:
image

name='depth',
label='Depth Maps',
description='Generated depth maps.',
semantic='image',
Copy link
Member

Choose a reason for hiding this comment

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

semantic=desc.Semantic.Image,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same answer as above : #1776 (comment)

@fabiencastan
Copy link
Member

Maybe add "panoramicImage" for PanoramaMerging output that can be loaded in 2D and 3D.

@fabiencastan
Copy link
Member

fabiencastan commented Sep 22, 2022

Need to check that the node is computed to load it in the 2D viewer (or at least running?).

@fabiencastan
Copy link
Member

fabiencastan commented Sep 22, 2022

If there is no param of image type to a node, it should not replace the last active node of the 2D Viewer.
For instance, loading the Meshing node in the 3D Viewer should not unload the active node of the 2D Viewer, same for FeatureMatching, etc.
But we also need a way to unload it to see the original images from the ImageGallery.

@fabiencastan
Copy link
Member

Not specific to the PR, it's an historical problem: when changing the selection in the ImageGallery using the keyboard, it would be good to update the UI accordingly, as it's done when changing the selected image with the mouse.

@cbentejac
Copy link
Contributor

If there is no param of image type to a node, it should not replace the last active node of the 2D Viewer. For instance, loading the Meshing node in the 3D Viewer should not unload the active node of the 2D Viewer, same for FeatureMatching, etc. But we also need a way to unload it to see the original images from the ImageGallery.

I am not sure of how we could make such a behaviour user-friendly...
Right now, if you want to switch to the images from the ImageGallery while something's already loaded, you have to unload the node (by "loading" another node that doesn't output images). If loading a node in the 3D viewer shouldn't unload the current 2D viewer node (unless it also has 2D ouputs), and we shouldn't replace it by a node that has no output, then what could unload it (and how)?
Maybe this could be avoided by adding a classic "image" choice in the combo box for nodes that can be loaded in the 2D viewer:

  • if no node has been loaded in the 2D viewer yet, then images from the ImageGallery are displayed as usual
  • if a node has been loaded in the 2D viewer (let's say a DepthMap node), then the user can chose to see the depth, sim or classic image from the ImageGallery with the combo box
  • if a node is loaded in the 3D viewer (with no 2D outputs) while another is already loaded in the 2D viewer, then it is correctly loaded in the 3D viewer and nothing happens in the 2D viewer (and the "image" choice in the combo box is still available to display the regular images)
  • if a node is loaded in the 2D viewer and another node that has no output is double-clicked, then nothing happens

The issue is that there won't be any way to unload a node without replacing it by another.

meshroom/ui/qml/Viewer/Viewer2D.qml Outdated Show resolved Hide resolved
@@ -990,18 +1043,18 @@ FocusScope {
}

ComboBox {
id: imageType
id: outputAttribute
property var activeNode: root.oiioPluginAvailable ? _reconstruction.activeNodes.get('allDepthMap').node : null
Copy link
Contributor

Choose a reason for hiding this comment

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

You removed the "imageType" ID as well as references to it everywhere but at line 563. It generates a "Viewer2D.qml:563: ReferenceError: imageType is not defined" warning when Meshroom is started.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point sorry about that ^^

This raises a question though : this depthMapNodeName Label doesn't really make much sense now since we don't display just depth maps but any sequence of output images related to the viewpoints, and in addition users should be aware of what is being displayed since you actually need to double click a node to visualize it, so shouldn't we just remove that Label ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It took me a little time to find where this label was displayed (I had to add the red background shown in the image below to find it...), but now that I see it, I think that it should definitely be kept but renamed on the QML side.
It is not specific to the depth maps anymore, but since it displays the name of the node that's currently loaded in the viewer, it is quite useful. Considering that double-clicking on a node might not necessarily change the node loaded in the 2D viewer, it's a good way to keep track of what's shown in there.
image

meshroom/ui/qml/main.qml Outdated Show resolved Hide resolved
@fabiencastan
Copy link
Member

The issue is that there won't be any way to unload a node without replacing it by another.

Yes, but IMH that's not a problem as long as we can still display the source image as you suggested.
BTW I have not double checked, if the behavior is fine if we remove the active node from the Graph.

@fabiencastan
Copy link
Member

Detail: maybe good to add mask and weight to PanoramaWarping.

@fabiencastan
Copy link
Member

We need to keep the capability to drag&drop external images into the 2D Viewer.

@fabiencastan
Copy link
Member

In the combobox, use the Label instead of the technical name of the attribute to match the information displayed in the NodeEditor.

@fabiencastan
Copy link
Member

Need to be able to load the node when we have chunks with mixed status:
image

@fabiencastan
Copy link
Member

Double click on a node when already on "Gallery" should switch to the first element of the combobox again.
Currently it's doing if I am on DepthMap node, switch to gallery, then double clic on PrepareDenseScene.
But it's not doing it if I'm on DepthMap node and switch to DepthMapFilter node (as the list of output is the same.

BUT it's a good think that I can be on "Sim Maps" and switch between DepthMap node and DepthMapFilter node.

@fabiencastan
Copy link
Member

Maybe rename "Gallery" as "Image Gallery" in the combo box.
And also update the label of the Image Gallery widget from "Images" to "Image Gallery".

@fabiencastan
Copy link
Member

fabiencastan commented Sep 29, 2022

BTW could you update the labels of the output params of PanoramaMerging from "Output Folder" to "Panorama"?
And remove the "Output" prefix from all the labels (mainly in the Panorama pipeline).

@fabiencastan
Copy link
Member

Maybe good also to add a close button at the right of the node name to remove it from the 2D Viewer:
image

@mugulmd
Copy link
Contributor Author

mugulmd commented Sep 30, 2022

Double click on a node when already on "Gallery" should switch to the first element of the combobox again. Currently it's doing if I am on DepthMap node, switch to gallery, then double clic on PrepareDenseScene. But it's not doing it if I'm on DepthMap node and switch to DepthMapFilter node (as the list of output is the same.

BUT it's a good think that I can be on "Sim Maps" and switch between DepthMap node and DepthMapFilter node.

So should we keep the current behavior or change to always re-initialize the ComboBox index when we change node ?

It seems like the current behavior can be useful for comparing the output of DepthMap and DepthMapFilter without clicking too many things, and it doesn't impact anything else, so maybe keep it (for now at least) ?

@mugulmd
Copy link
Contributor Author

mugulmd commented Sep 30, 2022

Maybe good also to add a close button at the right of the node name to remove it from the 2D Viewer: image

There's already a menu item in the image viewer header to toggle on/off the visibility of the node name and image filepath, so that would be a bit redundant don't you think ?

@cbentejac
Copy link
Contributor

cbentejac commented Oct 4, 2022

There's already a menu item in the image viewer header to toggle on/off the visibility of the node name and image filepath, so that would be a bit redundant don't you think ?

I think the "close" button would unload the node from the viewer, not hide the node's name and path.

mugulmd and others added 26 commits October 19, 2022 09:50
@fcastan fcastan force-pushed the dev/lv/adapt2DViewerToNode branch from 4c01703 to 0424ae1 Compare October 19, 2022 08:56
@fabiencastan fabiencastan merged commit 1f78158 into develop Oct 19, 2022
@fabiencastan fabiencastan deleted the dev/lv/adapt2DViewerToNode branch October 19, 2022 14:16
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.

3 participants