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

texture: add image mask support #967

Merged
merged 16 commits into from
May 20, 2023
Merged

Conversation

Sanheiii
Copy link
Contributor

@Sanheiii Sanheiii commented Apr 4, 2023

Masking would not be used in TextureMesh, now I implemented it.
Masks are sometimes not binarized, so I think we should exclude all values that are not greater than nIgnoreMaskLabel.

@ghost ghost mentioned this pull request Apr 4, 2023
@Sanheiii
Copy link
Contributor Author

Sanheiii commented Apr 4, 2023

Sorry, I found that in some cases it still throws an exception, I will try to fix it.

When I introduced the mask for TextureMesh I accidentally modified part of the code of omp, which caused the program to throw an exception, now I fixed it.
@Sanheiii
Copy link
Contributor Author

Sanheiii commented Apr 6, 2023

I fixed the above problem and tested it, and it works fine now.

@@ -535,6 +542,9 @@ bool MeshTexture::ListCameraFaces(FaceDataViewArr& facesDatas, float fOutlierThr
ASSERT((idxFace == NO_ID && depthMap(j,i) == 0) || (idxFace != NO_ID && depthMap(j,i) > 0));
if (idxFace == NO_ID)
continue;
if (!mask.empty() && !mask.isSet(j, i)) {
Copy link
Owner

Choose a reason for hiding this comment

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

this is not ok, there will be cases where the face partially overlaps the invalid region of the mask, so the few pixels that remain valid will assign the view to the mask even though the face contains invalid pixels; pls see PR #968 for a correct implementation; pls build on top of it; if so, pls move the mask estimation in that PR ourside under a command line paramter, to be estimated if the scene does not contain mask files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I understand the right way to implement it, I will finish it.

@cdcseacave
Copy link
Owner

looking good, great work, I still wait for the #968 PR to be fixed, and I'll let you know when it is merged

@EliDavis3D
Copy link

It looks like #968 is merged, does that mean this is valid now?

@cdcseacave
Copy link
Owner

@Sanheiii thx for this PR; I modified it a bit though: I removed all mask processing (should be done outside OpenMVS, when the masks are produced, including merging more labels into one, and erosion, etc), and unified the mask loading and usage

@cdcseacave cdcseacave merged commit c69bae4 into cdcseacave:develop May 20, 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.

4 participants