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

Added descriptions to StructureFromMotion node #1932

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jaggzh
Copy link

@jaggzh jaggzh commented Mar 14, 2023

  • I have read the contribution guidelines
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere (actually I've been having a lot of difficulties building AliceVision.. so I'm working on that).
  • I have followed the prevailing code style (for history readability and limit conflicts for maintainance).

Description

I figured the descriptions could be much more helpful to users. I began with just the tooltip versions. I do NOT know if they're accurate so please review.

Features list

  • More detailed descriptions in StructureFromMotionNode

Implementation remarks

Used chatgpt to help! But did a lot of work on it myself (and a lot of back-and-forth), and then hand editing afterwards too.

@fabiencastan fabiencastan marked this pull request as draft March 18, 2023 13:47
@jaggzh jaggzh marked this pull request as ready for review March 18, 2023 18:36
@jaggzh jaggzh changed the title [WIP] Added descriptions to StructureFromMotion node Added descriptions to StructureFromMotion node Mar 26, 2023
@jaggzh
Copy link
Author

jaggzh commented Jun 20, 2023

Is this one complex to review? :}}

Copy link
Member

@simogasp simogasp left a comment

Choose a reason for hiding this comment

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

thanks for the contribution.
I left some comments

Comment on lines +99 to +117
description='The algorithm used to extract distinctive features from images.\n'
'sift: Popular method for detecting/describing features.\n'
' Robust to changes in scale and orientation\n'
' sift_float: Uses floats (not ints).\n'
' sift_upright: Variation that does not compute scale and orientation.\n'
' dspsift: Dense version of SIFT that samples features at regular intervals.\n'
' (More efficient, but may be less accurate.)\n'
'akaze: A faster and more memory-efficient alternative to SIFT.\n'
' Less sensitive to affine distortion, making it useful for changing camera perspectives.\n'
' (Good to use in combination with SIFT)\n'
' akaze_liop: With Local Intensity Order Pattern features.\n'
' akaze_mldb: With Multi-Learning Decision Binary features.\n'
'cctag3: Features for detecting Cube-Circle-Triangle markers in images.\n'
' Robust to changes in illumination and scale, and can be used to estimate camera pose.\n'
'cctag4: Variation of CCTAG with more information in markers.\n'
'sift_ocv: Implementation of SIFT provided by OpenCV.\n'
'akaze_ocv: Implementation of AKAZE provided by OpenCV.\n'
'tag16h5: Features for detecting AprilTag markers in images.\n'
' Robust and reliable for object detection and pose estimation.',
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather integrate this into the FeatureExtraction node as here it is only used to select which type of feature that has been already extracted to use for the SfM part

' (Good to use in combination with SIFT)\n'
' akaze_liop: With Local Intensity Order Pattern features.\n'
' akaze_mldb: With Multi-Learning Decision Binary features.\n'
'cctag3: Features for detecting Cube-Circle-Triangle markers in images.\n'
Copy link
Member

Choose a reason for hiding this comment

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

FYI CCTag = Concentric Circles Tag ;-)

@@ -220,7 +254,10 @@ class StructureFromMotion(desc.AVCommandLineNode):
desc.FloatParam(
name='minAngleForLandmark',
label='Min Angle For Landmark',
description='Minimum angle for landmark.',
description='This parameter sets the minimum angle required between two views to create a landmark. A landmark is '
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description='This parameter sets the minimum angle required between two views to create a landmark. A landmark is '
description='This parameter sets the minimum angle required between two rays in space to create a 3D landmark. A landmark is '

Comment on lines +258 to +260
'a point that is manually marked in multiple images to help improve the reconstruction accuracy. '
'A higher value for this parameter can reduce the number of landmarks created, but may also improve '
'the overall accuracy of the reconstruction.',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'a point that is manually marked in multiple images to help improve the reconstruction accuracy. '
'A higher value for this parameter can reduce the number of landmarks created, but may also improve '
'the overall accuracy of the reconstruction.',
'a 3D point obtained by triangulating corresponding features in two or more images. '
'A higher value for this parameter can reduce the number of landmarks created, but may also improve '
'the overall accuracy of the reconstruction.',

Comment on lines +315 to +316
'Rig constraints are used to enforce certain geometric relationships between the cameras in a '
'multi-camera system.',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'Rig constraints are used to enforce certain geometric relationships between the cameras in a '
'multi-camera system.',
'Rig constraints are used to enforce a rigid geometric relationships between cameras that are part of the same multi-camera system.',

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