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

Rename markers to print #165

Merged
merged 2 commits into from
Jul 21, 2021

Conversation

arcdev
Copy link
Contributor

@arcdev arcdev commented Jun 29, 2021

Description

The CCTag markersToPrint files start with number 00001 however Meshroom's first CCTag is number 0.

Features list

Implementation remarks

Following the issue #141, update the generate.py to start with 0 rather than 1.
Also, removed & regenerated the existing PDF files.

Note: originally the files appear to have been generated with 5 placeholders, but the existing generate.py file uses a zfill(4) so, in order to make the fewest changes to the actual generator I opted to leave it at 4 placeholders.
I'm happy to change the generate.py file's zfill to 5 and regenerate the PDF files, if that's perferred.

Personally, I'd prefer to provide generated PDFs with the cross in place to ease measuring the distance. Seems like this should be the default, but I don't feel strongly.

@simogasp simogasp added this to the v1.1.0 milestone Jun 29, 2021
@simogasp simogasp self-assigned this Jun 29, 2021
@simogasp simogasp requested a review from lcalvet June 29, 2021 09:07
@simogasp
Copy link
Member

simogasp commented Jun 29, 2021

Thanks for this PR!

For me it is ok for all the points you mentioned

  • zfill(4) is enough because we cannot have more than 9999 markers anyway
  • conceptually I'm all in to not have the pdf, png etc since there is a generator. But I don't know if for people less familiar with using and launching python scripts that would be a barrier (I'm thinking of people with no dev background using Meshroom just to generate 3D models) Maybe it will be better to have the pdf, not that they will change anytime soon anyway.
  • I'm also ok with the cross and the id by default, if this does not create a problem at detection,

For the last point and for moving to zero-based id I let @lcalvet and @griwodz have the final say as there may be a reason for having the 1-based id (even if in the code they are zero-based).

@arcdev
Copy link
Contributor Author

arcdev commented Jun 30, 2021

Thanks for this PR!

For me it is ok for all the points you mentioned

  • zfill(4) is enough because we cannot have more than 9999 markers anyway
  • conceptually I'm all in to not have the pdf, png etc since there is a generator. But I don't know if for people less familiar with using and launching python scripts that would be a barrier (I'm thinking of people with no dev background using Meshroom just to generate 3D models) Maybe it will be better to have the pdf, not that they will change anytime soon anyway.
  • I'm also ok with the cross and the id by default, if this does not create a problem at detection,

For the last point and for moving to zero-based id I let @lcalvet and @griwodz have the final say as there may be a reason for having the 1-based id (even if in the code they are zero-based).

Sure thing!
I stumbled across this project trying do some photogrammetry for a part for a friend's Harley.

Personally, I like the idea of having a set of provided assets (pdf, png, svg, etc.). I came here and "just wanted" the things I needed. Since they really don't change, I'm thinking there shouldn't be much need for someone to regenerate them. It's good to have the generator if/when we do need to, but... why make everyone do it themselves?

I'm a developer (more than 20 years), but I focus on .NET & web techs, so firing up python always takes me a bit to get remember. :-)

I'm perfectly okay with whatever decision is best for the project, though I'd say that having the assets pre-generated removes a barrier for folks.

I also know there's a doc in one of the wiki pages that specifically calls out the 0 vs 1 so I can find and update that, if we proceed with the 0-based PDFs files.

@fabiencastan
Copy link
Member

IMO:

  • Padding of 4 is good.
  • We should keep the generated images.

So the PR looks good to me.
@arcdev @simogasp : Is it ready to merge?

@fabiencastan fabiencastan merged commit 7f57821 into alicevision:develop Jul 21, 2021
@simogasp simogasp modified the milestones: v1.1.0, v1.0.1 Aug 13, 2021
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.

[question] Which MarkerId to use in Meshroom SfMTransform node
3 participants