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

Native pointer is not assigned when creating packet with default constructor #54

Closed
ronhuang opened this issue Apr 14, 2023 · 3 comments · Fixed by #55
Closed

Native pointer is not assigned when creating packet with default constructor #54

ronhuang opened this issue Apr 14, 2023 · 3 comments · Fixed by #55
Assignees
Labels
bug Something isn't working help wanted Extra attention is needed priority:high

Comments

@ronhuang
Copy link

Thank you for porting this library from MediaPipeUnityPlugin. Wonderful work.

I noticed that there are mismatches in the default constructors of various Packet subclasses (e.g., DetectionVectorPacket, ImageFramePacket, etc.) between this project and MediaPipeUnityPlugin. In MediaPipeUnityPlugin, the default constructors call the base(true) base constructor, while the constructors in this project call base().

Using DetectionVectorPacket as example. The MediaPipeUnityPlugin project has the following.
https://github.com/homuler/MediaPipeUnityPlugin/blob/v0.9.1/Packages/com.github.homuler.mediapipe/Runtime/Scripts/Framework/Packet/DetectionVectorPacket.cs#L17

    public DetectionVectorPacket() : base(true) { }

And DetectionVectorPacket in this project has the following.
https://github.com/vignetteapp/MediaPipe.NET/blob/176415561c442cce8e92d1cde59b875e6a14995a/Mediapipe.Net/Framework/Packets/DetectionVectorPacket.cs#L15

Any particular reason on calling a different base constructor? If there is not any, can you please update default constructors of all Packet subclasses to call base(true) instead? Without base(true), native pointer to Packet is never assigned.

https://github.com/vignetteapp/MediaPipe.NET/blob/176415561c442cce8e92d1cde59b875e6a14995a/Mediapipe.Net/Framework/Packets/Packet.cs#L16-L26

Also, if it is not too much trouble, I would really appreciate if you can upload new NuGet packages with this change. Thank you so much.

@sr229 sr229 added bug Something isn't working help wanted Extra attention is needed priority:high labels Apr 15, 2023
@sr229
Copy link
Member

sr229 commented Apr 15, 2023

MediaPipe.NET was based off an earlier version of MediaPipeUnity, so things may have changed quite a bit. As for the other discrepancies like mismatching classes, I'll go ahead and fix them right now.

@sr229
Copy link
Member

sr229 commented Apr 16, 2023

This should be fixed in 0.9.2 now, please let me know if there's still some outliers.

@ronhuang
Copy link
Author

0.9.2 works perfectly. Thank you for the quick fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed priority:high
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants