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

DensifyPointCloud: --mask-path to specify folder containing mask images #940

Merged
merged 2 commits into from
Mar 17, 2023

Conversation

kenshi84
Copy link
Contributor

No description provided.

@ghost
Copy link

ghost commented Feb 16, 2023

Don't think it should be called "ignore-mask-path" as it makes it sound the path is being ignored.

@kenshi84 kenshi84 changed the title DensifyPointCloud: add --ignore-mask-path arg to easily specify mask images DensifyPointCloud: add --mask-path arg to easily specify mask images Feb 17, 2023
@kenshi84
Copy link
Contributor Author

kenshi84 commented Feb 17, 2023

@4CJ7T
Thanks for your feedback, I modified the patch accordingly.

I chose that name because of the --ignore-mask-label arg name and function names ApplyIgnoreMask & ImportIgnoreMask in DepthMap.cpp which made me think that "ignore mask" is some special term in this context. But I agree, just simply using "mask" makes sense and should suffice.

Copy link
Owner

@cdcseacave cdcseacave left a comment

Choose a reason for hiding this comment

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

LGTM, I will merge it after you point it to develop instead of master

VERBOSE("Image %s has non-empty maskName %s", image.name.c_str(), image.maskName.c_str());
return EXIT_FAILURE;
}
image.maskName = Util::ensureFolderSlash(OPT::strMaskPath) + Util::getFileNameExt(image.name);
Copy link
Owner

Choose a reason for hiding this comment

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

you could also add here a check that the composed mask file name actually exists

@kenshi84
Copy link
Contributor Author

Rebased onto the develop branch, which made me aware of this patch 2874f19 which offers a simple way to specify mask images. Given this patch, I decided to use the same file extension (.mask.png) to avoid chance of confusion by the user. Also added the file existence check.

@kenshi84 kenshi84 changed the title DensifyPointCloud: add --mask-path arg to easily specify mask images DensifyPointCloud: --mask-path to specify folder containing mask images Mar 14, 2023
@@ -296,6 +298,23 @@ int main(int argc, LPCTSTR* argv)
// load and estimate a dense point-cloud
if (!scene.Load(MAKE_PATH_SAFE(OPT::strInputFileName)))
return EXIT_FAILURE;
if (!OPT::strMaskPath.empty()) {
if (OPTDENSE::nIgnoreMaskLabel < 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

this check is probably not needed, as all you do here is setting the maskName variable, which will not be used if not requested; however can be saved and added to the MVS scene file; what do you think?

VERBOSE("Image %s has non-empty maskName %s", image.name.c_str(), image.maskName.c_str());
return EXIT_FAILURE;
}
image.maskName = Util::ensureFolderSlash(OPT::strMaskPath) + Util::getFileName(image.name) + ".mask.png";
Copy link
Owner

Choose a reason for hiding this comment

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

Util::ensureFolderSlash(OPT::strMaskPath) needs to be run once at the beginning only; and you should use ensureValidFolderPath instead, which does more checks

}
image.maskName = Util::ensureFolderSlash(OPT::strMaskPath) + Util::getFileName(image.name) + ".mask.png";
if (!File::access(image.maskName)) {
VERBOSE("Mask image %s not found in %s", image.maskName.c_str(), OPT::strMaskPath.c_str());
Copy link
Owner

Choose a reason for hiding this comment

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

do not print strMaskPath as that is included in the maskName now

@kenshi84
Copy link
Contributor Author

@cdcseacave Thanks for your feedback, modified the patch accordingly.

@cdcseacave cdcseacave self-requested a review March 17, 2023 08:46
Copy link
Owner

@cdcseacave cdcseacave left a comment

Choose a reason for hiding this comment

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

Thank you, looking good!

@cdcseacave cdcseacave merged commit 97776fe into cdcseacave:develop Mar 17, 2023
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