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

Reuse p and R memory from params in apriltag_pose #197

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

Conversation

Affie
Copy link
Contributor

@Affie Affie commented Jan 25, 2022

Hi, I want to update the Julia wrapper for apriltags and I have this change that fixes an issue. Memory not owned by the function is freed and new memory is allocated.
I don't know if this is the best way and I'm open to something better as long as the memory passed in is not freed in matd_destroy.

Thanks

@christian-rauch
Copy link
Collaborator

What is the motivation behind this change? The memory for t is freed and allocated again. Is this an issue? Alternatively, you are allocating and freeing tmp1, which probably has the same cost.

@Affie
Copy link
Contributor Author

Affie commented Jan 25, 2022

In the julia wrapper the memory is managed by julia and freeing it in the orthogonal_iteration function causes a segfault.
So it's not about the cost.

@Affie
Copy link
Contributor Author

Affie commented Jan 25, 2022

Note, I call orthogonal_iteration directly to save on converting between julia and c types.

@Affie
Copy link
Contributor Author

Affie commented Feb 4, 2022

Also note: we build binaries for easy installation and use with Julia https://github.com/JuliaBinaryWrappers/AprilTags_jll.jl/releases/tag/AprilTags-v0.1.0%2B0, it is built for all supported platforms, but I haven't tested it on other platforms. I would like to make maintenance easier and directly build from this repository, it's currently built from a fork.

@christian-rauch
Copy link
Collaborator

@mkrogius @wxmerkt If you don't know of a better way to set the matrix memory directly without creating another matrix (tmp1 and tmp2), I would go ahead with this proposal.

@wxmerkt
Copy link
Collaborator

wxmerkt commented Feb 4, 2022

@christian-rauch I am afraid I can't comment on this. @mkrogius is the best person to answer

@mkrogius
Copy link
Contributor

mkrogius commented Feb 5, 2022

I'm leaning against accepting this change because somebody reading this code will not understand why this code is written this way. Surely there must be some way in the Julia wrapper to create and pass in a "t" that is a native c type and so would not segfault?

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